feat: add review panel git, branch, and turn diff modes#344
feat: add review panel git, branch, and turn diff modes#344
Conversation
There was a problem hiding this comment.
Pull request overview
Adds review-panel diff mode switching so users can view changes at different scopes (session, git working tree, branch, and per-turn), persisting the chosen mode in layout state.
Changes:
- Add
reviewModeto layout context state with localStorage persistence. - Add a review-mode selector UI to the review panel and load diffs based on the selected mode.
- Add
Vcs.diff()to the generated SDK for/vcs/diffwith amodequery parameter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app-prefixable/src/sdk/gen/sdk.gen.ts | Adds Vcs.diff() method to call /vcs/diff for git/branch diffs. |
| app-prefixable/src/context/layout.tsx | Introduces ReviewMode and persists selected mode via layout storage. |
| app-prefixable/src/components/review-panel.tsx | Adds mode selector UI and mode-specific diff loading (session/git/branch/turn). |
Comments suppressed due to low confidence (1)
app-prefixable/src/components/review-panel.tsx:170
- The createEffect that triggers loadDiffs() resets
selectedon every re-run. Since the effect now depends onmode()(and onlastUserMessageId()in turn mode), switching modes or getting a new turn will clear the current file selection and force re-selection of the first diff viasetFiles(), which is a UX regression. Consider only resetting selection whenprops.sessionIdchanges (e.g., track previous sessionId or useon(() => props.sessionId, ...)), and keep the existing selection across mode changes when the file still exists in the new diff set.
// 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);
loadDiffs();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** | ||
| * 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.
sdk.gen.ts is marked as auto-generated. Adding Vcs.diff() directly here means the method will be lost the next time the SDK is regenerated, and it can also drift from the actual OpenAPI contract. If possible, update the OpenAPI spec (or the generation inputs) to include /vcs/diff and regenerate so the change is durable and the corresponding request/response types are emitted.
| /** | |
| * 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, | |
| }) | |
| } |
| if (currentMode === "git" || currentMode === "branch") { | ||
| const res = await client.vcs.diff({ directory, mode: currentMode }); | ||
| if (current !== version) return; |
There was a problem hiding this comment.
This introduces a new dependency on the /vcs/diff endpoint for git/branch modes, but the existing API contract smoke tests don’t cover any VCS endpoints. Consider adding a lightweight contract test for GET /vcs/diff?mode=git (and/or mode=branch) to catch missing/renamed endpoints early, similar to the other endpoint availability checks in tests/api-contract.test.ts.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app-prefixable/src/components/review-panel.tsx:173
- The effect that reloads diffs depends on
mode()(andlastUserMessageId()in turn mode), but it always resetsselectedto null. This means switching review modes (or a new user message arriving in turn mode) will clear the current file selection even when the selected file still exists in the new diff set. Consider only clearing selection whenprops.sessionIdchanges, and otherwise preserve selection (yoursetFiles()helper already corrects invalid selections).
// 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);
loadDiffs();
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return (options?.client ?? this.client).get<SessionDiffResponses, unknown, ThrowOnError>({ | ||
| url: "/vcs/diff", | ||
| ...options, |
There was a problem hiding this comment.
Vcs.diff() currently reuses SessionDiffResponses as its response type. Even if the payload shape is identical, this couples two unrelated endpoints and can be confusing for SDK consumers. Consider introducing a dedicated VcsDiffResponses type (e.g., 200: Array<FileDiff>) and using it here for clarity and stronger API semantics.
There was a problem hiding this comment.
Fixed in 4a65d52. Added dedicated VcsDiffResponses/VcsDiffResponse in types.gen.ts and updated Vcs.diff() to use VcsDiffResponses instead of SessionDiffResponses for clearer API semantics.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app-prefixable/src/components/review-panel.tsx:173
createEffectthat loads diffs now depends onmode()and (in turn mode)lastUserMessageId(), but it resetsselectedtonullon every re-run. This means switching diff mode (or when the last user message changes) will clear the user’s file selection, even though the comment says it only resets when the session changes. Consider splitting the effect so selection is reset only whenprops.sessionIdchanges, while mode/turn changes just callloadDiffs()(and optionally keep the selection if still present).
// 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);
loadDiffs();
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type VcsDiffResponses = { | ||
| /** | ||
| * Diff | ||
| */ | ||
| 200: Array<FileDiff> | ||
| } | ||
|
|
||
| export type VcsDiffResponse = VcsDiffResponses[keyof VcsDiffResponses] |
There was a problem hiding this comment.
types.gen.ts defines VcsDiffResponses/VcsDiffResponse, but unlike other endpoints (e.g. VcsGetData, SessionDiffData) there is no corresponding VcsDiffData type describing the request (query params like directory and mode). This makes the generated API surface inconsistent and can break downstream code that relies on the *Data types. Consider adding a VcsDiffData type (and including mode in its query), ideally matching the OpenAPI spec so future regenerations don’t drop it.
There was a problem hiding this comment.
Implemented fix in app-prefixable/src/sdk/gen/types.gen.ts by adding missing VcsDiffData with directory and mode query params plus /vcs/diff URL, matching the surrounding *Data request type pattern. No SDK call-site shape changes were required; existing vcs.diff() parameters already align. Included in commit 6e26826.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app-prefixable/src/components/review-panel.tsx:172
createEffectresetsselectedany time the effect re-runs (mode changes / turn dependency changes), but the inline comment says it's only resetting on session changes. Either update the comment to match the behavior or gate the reset so it only happens whenprops.sessionIdactually changes (e.g., track previous sessionId).
// 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);
loadDiffs();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 }); |
There was a problem hiding this comment.
isGitRepo can retain a stale false value from a prior load/mode, which can briefly render the “Not a Git repository” empty state immediately after switching into git/branch mode (before loadDiffs() sets loading/recomputes repo status). Consider resetting isGitRepo to null at the start of loadDiffs() (or inside setMode) when entering git/branch modes so the UI doesn’t display an out-of-date repo state.
There was a problem hiding this comment.
Fixed in 4b73df9. I now reset isGitRepo to null at the start of loadDiffs() when entering git/branch mode, so stale false does not briefly render the "Not a Git repository" state during mode switches.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app-prefixable/src/components/review-panel.tsx:173
createEffectnow depends onmode()(andlastUserMessageId()in turn mode), but it unconditionally callssetSelected(null)with a comment implying this only happens on session changes. This means switching diff modes (or a new user message arriving in turn mode) will clear the current file selection even if that file still exists in the next diff set. Consider resetting selection only whenprops.sessionIdactually changes (e.g., usingon(() => props.sessionId, ...)or tracking the previous id), and rely onsetFiles()to adjust selection when the chosen file disappears.
// 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);
loadDiffs();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try to get VCS info - if it fails or returns no branch, it's not a git repo | ||
| const res = await client.vcs.get({ directory }); | ||
| if (current !== version) return; | ||
| setIsGitRepo(res.data?.branch !== undefined); |
There was a problem hiding this comment.
The comment in checkGitRepo says "if it fails or returns no branch", but VcsInfo.branch is a required string in the generated types. To avoid misleading future readers, consider updating the comment to reflect the actual check being performed (i.e., "if the request fails, treat it as not a Git repo"), or adjust the condition if the backend can legitimately return an empty/absent branch.
| // Try to get VCS info - if it fails or returns no branch, it's not a git repo | |
| const res = await client.vcs.get({ directory }); | |
| if (current !== version) return; | |
| setIsGitRepo(res.data?.branch !== undefined); | |
| // If the VCS info request succeeds, treat the directory as a Git repo. | |
| await client.vcs.get({ directory }); | |
| if (current !== version) return; | |
| setIsGitRepo(true); |
There was a problem hiding this comment.
Fixed in 6630c14. Updated checkGitRepo to treat a successful client.vcs.get call as definitive git-repo detection, and adjusted the comment accordingly (removed the branch-presence check since VcsInfo.branch is required by contract).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function checkGitRepo(current: number) { | ||
| try { | ||
| // Try to get VCS info - if it fails or returns no branch, it's not a git repo | ||
| const res = await client.vcs.get({ directory }); | ||
| setIsGitRepo(res.data?.branch !== undefined); | ||
| // If VCS info request succeeds, the directory is a git repository | ||
| await client.vcs.get({ directory }); | ||
| if (current !== version) return; | ||
| setIsGitRepo(true); | ||
| } catch { | ||
| if (current !== version) return; | ||
| setIsGitRepo(false); | ||
| } |
There was a problem hiding this comment.
checkGitRepo() treats any failure of client.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 calling client.vcs.get with throwOnError: false (or using a non-throwing client) and only setting isGitRepo(false) for the specific status/error that indicates “not a git repo”; otherwise keep isGitRepo(null) and surface a generic load error state.
There was a problem hiding this comment.
Fixed in b3234eb. I switched checkGitRepo() to use a non-throwing client.vcs.get({ directory }, { throwOnError: false }) call, classify explicit not-a-git errors via message matching, and only set isGitRepo(false) for that case. For transient/auth/server/network failures, it now keeps isGitRepo(null) so the panel does not incorrectly render the "Not a Git repository" empty state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
app-prefixable/src/components/review-panel.tsx:190
- The comment says the selection is reset “when session changes”, but this effect also reruns on review mode changes (and on
lastUserMessageIdchanges in turn mode) and will reset the selection in those cases too. Update the comment to reflect the broader trigger, or change the logic to only reset selection whenprops.sessionIdactually changes.
// Reset selection when session changes
setSelected(null);
loadDiffs();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #230