fix: open a commit diff for any sha - #4468
Conversation
|
| Filename | Overview |
|---|---|
| packages/app/src/git/use-diff-files.ts | Replaces bounded commit-list lookup with a capability-gated per-SHA query, but conflates unloaded server metadata with a confirmed missing capability. |
| packages/protocol/src/messages.ts | Adds validated commit-files request/response messages and the optional server capability flag. |
| packages/server/src/server/session/checkout/checkout-session.ts | Handles the new request with revision validation, file lookup, and structured error responses. |
| packages/server/src/utils/checkout-git.ts | Adds the first-parent commit-file lookup; its public parameter still uses the inline type identified in the previous review. |
| packages/app/e2e/browser/commit-diff-aged-out.spec.ts | Exercises the user-visible regression by keeping a diff open after its commit leaves the bounded list. |
Sequence Diagram
sequenceDiagram
participant Panel as Commit diff panel
participant App as App query layer
participant Client as Daemon client
participant Session as Checkout session
participant Git as Git utilities
Panel->>App: Request files for SHA
App->>Client: getCommitFiles(cwd, sha)
Client->>Session: checkout.commits.files.request
Session->>Session: Validate SHA and workspace.read permission
Session->>Git: "getCommitFiles({ cwd, sha })"
Git-->>Session: First-parent file records
Session-->>Client: checkout.commits.files.response
Client-->>App: Files
App->>Client: Request each file diff
Client-->>Panel: Parsed file diffs
Reviews (2): Last reviewed commit: "fix: open a commit diff for any sha" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function getCommitFiles({ | ||
| cwd, | ||
| sha, | ||
| }: { | ||
| cwd: string; | ||
| sha: string; | ||
| }): Promise<CheckoutCommitFile[]> { | ||
| const records = await getCheckoutCommitRecords({ cwd, revision: sha, maxCount: 1 }); | ||
| return records[0]?.files ?? []; | ||
| } |
There was a problem hiding this comment.
The exported getCommitFiles function declares its multi-property parameter type inline. This violates the server guide’s requirement that public signatures use named types, so the repository requirement must be satisfied before merging.
| export async function getCommitFiles({ | |
| cwd, | |
| sha, | |
| }: { | |
| cwd: string; | |
| sha: string; | |
| }): Promise<CheckoutCommitFile[]> { | |
| const records = await getCheckoutCommitRecords({ cwd, revision: sha, maxCount: 1 }); | |
| return records[0]?.files ?? []; | |
| } | |
| interface GetCommitFilesOptions { | |
| cwd: string; | |
| sha: string; | |
| } | |
| export async function getCommitFiles({ | |
| cwd, | |
| sha, | |
| }: GetCommitFilesOptions): Promise<CheckoutCommitFile[]> { | |
| const records = await getCheckoutCommitRecords({ cwd, revision: sha, maxCount: 1 }); | |
| return records[0]?.files ?? []; | |
| } |
Context Used: packages/server/CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The commit diff panel took its file list from checkout.commits.list, which answers a different question: what this workspace changed relative to its base, plus CHECKOUT_BASE_COMMIT_LIMIT (10) commits of base history. A sha outside that window was absent from the list, so the panel rendered "No changes" for a commit that plainly has some. Nothing in the app could reach such a commit before, because the Commits section only offers the ones in the list. A plugin panel or a deep link can. Add checkout.commits.files, which lists one commit's files by sha, and read it from the panel instead of scanning the list. Merge commits report their first-parent diff, matching getCommitFileDiff. Gated once on the commitFiles capability.
4c5a951 to
74a78c0
Compare
| const capabilityPresent = useSessionStore( | ||
| (state) => state.sessions[serverId]?.serverInfo?.features?.commitFiles === true, | ||
| ); |
There was a problem hiding this comment.
When a retained commit-diff panel is restored during connection startup, serverInfo is still null, but this check treats that unknown state as a missing capability. Because the panel shows the capability warning before its loading state, users can briefly see “Update the host to view commit diffs” even when the daemon supports the feature. Distinguish metadata that has not loaded yet from metadata that has loaded without commitFiles.
1 participant
Linked issue
Closes #
Type of change
Reasoning
The commit diff panel took its file list from
checkout.commits.list. That RPC answers a different question: what this workspace changed relative to its base, plusCHECKOUT_BASE_COMMIT_LIMIT(10) commits of base history. A sha outside that window is simply absent from the response, souseCommitDiffFilesfound nothing and the panel rendered "No changes" for a commit that plainly has some.The window is small. On a branch four commits ahead of
main, everything older than the tenth commit onmainis affected — in a busy repo that is a few hours of history.You cannot reach such a commit from the Commits section today, because it only lists what the same RPC returned. You can reach it two other ways:
navigation.openCommitDifffor exactly that, and it is only useful if the panel can render an arbitrary sha.Goals
getCommitFileDiff.Non-goals
checkout.commits.listor to the Commits section. Paging older base history is a separate problem.Protocol compatibility
checkout.commits.files.request/.responseare new message types. An older app never sends the request, and never receives the response because it never asks. An older daemon parses nothing new.server_info.features.commitFilesis a new optional boolean. An older app ignores it; an older daemon omits it and a new app readsundefinedas absent.useCommitDiffFiles, and the panel already had acapabilityMissingstate that tells the user to update the host. No fallback path.// COMPAT(commitFiles): added in v0.8.0, remove gate after 2027-03-08.at the schema, the daemon flag, and the app gate.QA
Automated, on this branch:
Two new server cases in
checkout-git.commits.test.ts: a commit outside the commits list still reports its files, and a merge commit reports its first-parent diff.New browser regression in
packages/app/e2e/browser/commit-diff-aged-out.spec.ts. It opens a commit diff from the Commits section, adds twelve commits, movesmainup to the branch tip, hits Refresh, waits for "No commits ahead of main yet" (proving the commit really left the list), then asserts the open diff is still rendered.It fails on the old code for the reported reason. With
packages/app/src/git/use-diff-files.tsreverted tomain:and with the fix in place:
Same scenario, screenshotted. Before, once the commit leaves the list:
After:
Also verified against a real daemon with a plugin panel that opens arbitrary shas (the #4460 hook): a commit thirteen back from HEAD, well outside the window, renders its diff instead of "No changes".
Checklist
npm run typecheckpassesnpm run lintpassesnpm run formatpasses