feat(source-control): add stash actions to the commit dropdown - #11571
feat(source-control): add stash actions to the commit dropdown#11571Douglasgomes027 wants to merge 1 commit into
Conversation
c08237f to
cad5a06
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Git stash support across shared command execution, local and SSH providers, relay and runtime RPCs, preload APIs, and renderer clients. The Source Control sidebar now exposes stash, apply, pop, drop, and clear workflows with picker dialogs, state-aware submenu entries, confirmations, conflict handling, localization, and cache invalidation. Validation covers stash references, messages, commit identity guards, conflict outcomes, stale entries, real Git behavior, relay operations, RPC routing, renderer state, and end-to-end UI flows. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR introduces complete
Confidence Score: 5/5Safe to merge. All four transports share the same command core, ref injection is rejected at three independent layers, and the index-shift guard covers both picker and latest-entry flows. The only findings are style-level: an inconsistency in how stashDrop reads its ref param in the relay handler, and a timestamp freshness nit in the stash picker dialog. Neither affects correctness or safety. Files Needing Attention: src/relay/git-handler.ts — stashDrop uses params.ref as string instead of readStashRefParam, inconsistent with stashApply and stashPop but functionally safe due to downstream validation.
|
| Filename | Overview |
|---|---|
| src/shared/git-stash-commands.ts | New shared stash command core with injected executor, argument validation at all entry points, and the index-shift guard; used identically by local and relay hosts. |
| src/relay/git-handler.ts | Six relay stash handlers; apply/pop correctly use readStashRefParam but stashDrop casts params.ref directly — functionally safe but inconsistent with the helper pattern. |
| src/main/ipc/filesystem.ts | Six new IPC handlers routing to local git or SshGitProvider; worktreePath is resolved against the store on the local path. |
| src/main/providers/ssh-git-provider.ts | SSH stash methods delegate over the MUX with ref/oid omitted-not-null semantics; older-relay case surfaces a user-actionable reconnect error instead of a raw JSON-RPC -32601. |
| src/renderer/src/components/right-sidebar/useSourceControlStash.ts | Stash state hook; worktree-switch stale-count guard uses activeWorktreeIdRef correctly; facade is memoized; finishMutation is called in all paths. |
| src/renderer/src/components/right-sidebar/source-control-stash-menu-items.ts | Every row always renders with a reason; busy-beats-loading-beats-empty precedence is correctly enforced; untracked-only guard is explicit and tested. |
| src/renderer/src/components/right-sidebar/SourceControlStashPicker.tsx | Picker dialog loads fresh on each open; stale-flag cleanup is correct; nowMs is computed at render time so relative timestamps won't auto-update while the dialog is open. |
| src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts | trackedChangeCount / untrackedCount inputs correctly separate the two stash rows; isPullRequestOperationActive path now also disables submenu items, not just the trigger. |
Sequence Diagram
sequenceDiagram
participant R as Renderer (useSourceControlStash)
participant IPC as IPC / RPC Router
participant L as Local git/stash.ts
participant SSH as SshGitProvider
participant Relay as GitHandler (relay)
participant Shared as git-stash-commands.ts
R->>IPC: runStashAction(kind)
alt Local / WSL
IPC->>L: stashChanges / popStash / applyStash / dropStash / clearStashes
L->>Shared: stashChangesWith / popStashWith (injected exec)
Shared-->>L: GitStashPushResult / GitStashMutationResult
L-->>IPC: result
else SSH relay
IPC->>SSH: SshGitProvider.popStash / applyStash
SSH->>Relay: mux.request(git.stashPop)
Relay->>Shared: popStashWith (injected this.git)
Shared-->>Relay: result
Relay-->>SSH: result
SSH-->>IPC: result
else Remote runtime RPC
IPC->>Shared: RuntimeGitCommands popStash / SshGitProvider
Shared-->>IPC: result
end
IPC-->>R: result
R->>R: toast + finishMutation() + refreshStashCount()
Reviews (7): Last reviewed commit: "feat(source-control): add stash actions ..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/renderer/src/components/right-sidebar/SourceControlStashPicker.tsx (1)
38-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWheel-lock effect re-subscribes on every
entries.lengthchange; a mount-only effect would suffice.The handler reads
el.scrollHeight/el.clientHeightlive from the DOM on each wheel event, so it doesn't need to be re-created whenentrieschanges — an empty dependency array (attach once on mount, matching the ref's lifetime) would avoid the churn without behavior change.src/renderer/src/components/right-sidebar/source-control-stash-actions.ts (1)
9-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStash-kind membership logic is hand-duplicated instead of reusing the exported predicates.
isStashPickerActionandisStashActionare defined insource-control-stash-actions.tsspecifically to answer "is this DropdownActionKind a stash picker action / any stash action", but both call sites that need exactly this answer re-implement it inline. A new stash action kind added later would need updating in three places to stay correct.
src/renderer/src/components/right-sidebar/source-control-stash-actions.ts#L9-L15: no change needed here — this is the canonical definition the other two sites should call into.src/renderer/src/components/right-sidebar/useSourceControlStash.ts#L225-L238: replace the inlinekind === 'stash_pop_pick' || kind === 'stash_apply_pick' || kind === 'stash_drop_pick'check withisStashPickerAction(kind).src/renderer/src/components/right-sidebar/SourceControl.tsx#L4356-L4365: replace the 8 enumeratedcase 'stash...':labels with a singleisStashAction(kind)check (e.g. anifguard before/instead of the switch) so new stash kinds don't require touching this switch.src/renderer/src/components/right-sidebar/useSourceControlStash.ts (1)
219-241: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHook returns a brand-new object every render, undermining downstream memoization.
SourceControlStashis built as an object literal on every invocation (notuseMemo-wrapped), so its identity changes even when nothing stash-related changed (e.g. the caller re-rendered because the user typed in the commit textarea). Two consumers inSourceControl.tsx(handleDropdownOpenChange, deps[stash], andhandleActionInvoke, deps includesstash) depend on the whole object, so theiruseCallbackmemoization is effectively defeated on every keystroke/re-render — inconsistent with how heavily this surrounding component relies on stable callback identities elsewhere.♻️ Wrap the return value in useMemo
- return { - stashCount, - pickerMode, - closePicker: useCallback(() => setPickerMode(null), []), - refreshStashCount, - listEntries, - runStashAction: useCallback( - async (kind: DropdownActionKind) => { - if ( - kind === 'stash_pop_pick' || - kind === 'stash_apply_pick' || - kind === 'stash_drop_pick' - ) { - openPickerRef.current(kind) - return - } - await runStashAction(kind) - }, - [runStashAction] - ), - selectPickedStash - } + const closePicker = useCallback(() => setPickerMode(null), []) + const dispatchStashAction = useCallback( + async (kind: DropdownActionKind) => { + if (isStashPickerAction(kind)) { + setPickerMode(kind) + return + } + await runStashAction(kind) + }, + [runStashAction] + ) + return useMemo( + () => ({ + stashCount, + pickerMode, + closePicker, + refreshStashCount, + listEntries, + runStashAction: dispatchStashAction, + selectPickedStash + }), + [ + stashCount, + pickerMode, + closePicker, + refreshStashCount, + listEntries, + dispatchStashAction, + selectPickedStash + ] + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e3d1208-a3da-435b-9b1f-1f11252bec74
📒 Files selected for processing (42)
docs/reference/git-compatibility.mdsrc/main/git/stash-real-repo.test.tssrc/main/git/stash.test.tssrc/main/git/stash.tssrc/main/ipc/filesystem.tssrc/main/providers/git-stash-provider.tssrc/main/providers/ssh-git-provider.tssrc/main/providers/types.tssrc/main/runtime/orca-runtime-git.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/git-stash-params.tssrc/main/runtime/rpc/methods/git-stash.test.tssrc/main/runtime/rpc/methods/git.tssrc/main/runtime/runtime-rpc.tssrc/preload/api-types.tssrc/preload/index.tssrc/relay/git-handler-stash.test.tssrc/relay/git-handler.tssrc/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/SourceControlStashPicker.tsxsrc/renderer/src/components/right-sidebar/source-control-dropdown-entries.tsxsrc/renderer/src/components/right-sidebar/source-control-dropdown-items.test.tssrc/renderer/src/components/right-sidebar/source-control-dropdown-items.tssrc/renderer/src/components/right-sidebar/source-control-stash-actions.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.test.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/runtime/runtime-git-client.tssrc/renderer/src/web/web-preload-api.tssrc/shared/git-binary-compatibility.test.tssrc/shared/git-stash-commands.tssrc/shared/git-stash-conflict.tssrc/shared/git-stash-list-output.test.tssrc/shared/git-stash-list-output.tssrc/shared/git-stash-types.tstests/e2e/source-control-stash-actions.spec.ts
…e switch Review follow-ups on stablyai#11571: - refreshStashCount awaited without checking the worktree still matched, so a slow read for worktree A could overwrite the reset written on switch and leave B showing A's count. A non-zero stale count re-enables pop/apply/drop for a worktree with no stashes, so the row looks clickable and then errors. Compares against a live ref at response time — the closure's worktreeId is the one it was created for, so comparing that would always match. - useSourceControlStash returned a fresh object literal each render, making the consumer's useCallback a no-op. Memoized the facade and narrowed the call sites to the individual stable functions. - Moved the git:stash* arg aliases below the import block in filesystem.ts. Adds useSourceControlStash.test.ts covering the race, facade identity, the worktree-switch reset, picker dispatch, and the folder-workspace no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — all three findings were accurate and are fixed in 162b343. P1 — stale count survives worktree switch. Confirmed and fixed. Worth noting the suggested fix needed one adjustment: capturing P2 — unstable facade identity. Confirmed. Memoized the hook's return and narrowed the consumers to the individual stable functions ( P2 — type aliases between import blocks. Fixed, moved below all imports. Added One thing the identity test surfaced: the facade is only stable if the caller passes a stable Re-verified after the fixes: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/src/components/right-sidebar/SourceControl.tsx (1)
5850-5850: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRefresh stashes in the Create Review dropdown too.
Line 5850 wires refresh only to
CommitArea; the parallelCreateHostedReviewComposerreceives the samedropdownItemswithout this handler. When that composer is active, stash count remains unknown, leaving restore/drop actions disabled. PasshandleDropdownOpenChangethrough that dropdown path as well.src/renderer/src/components/right-sidebar/useSourceControlStash.ts (1)
99-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDiscard superseded refreshes for the same worktree.
The worktree check does not order overlapping requests. An initial menu refresh can resolve after a post-mutation refresh and overwrite the newer count, incorrectly enabling or disabling stash actions. Track a monotonic request/version token (also invalidated on worktree changes) and ignore older completions; add a reverse-completion test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de05dc85-5b1b-416c-bf75-58de3092cd51
📒 Files selected for processing (7)
src/main/ipc/filesystem.tssrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/useSourceControlStash.test.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.tstest-results/pr-11293-11177-integration-r1.mdtest-results/pr-11293-11177-integration-r2.mdtest-results/pr-11293-11177-integration-r3.md
💤 Files with no reviewable changes (3)
- test-results/pr-11293-11177-integration-r3.md
- test-results/pr-11293-11177-integration-r2.md
- test-results/pr-11293-11177-integration-r1.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/ipc/filesystem.ts
…e switch Review follow-ups on stablyai#11571: - refreshStashCount awaited without checking the worktree still matched, so a slow read for worktree A could overwrite the reset written on switch and leave B showing A's count. A non-zero stale count re-enables pop/apply/drop for a worktree with no stashes, so the row looks clickable and then errors. Compares against a live ref at response time — the closure's worktreeId is the one it was created for, so comparing that would always match. - useSourceControlStash returned a fresh object literal each render, making the consumer's useCallback a no-op. Memoized the facade and narrowed the call sites to the individual stable functions. - Moved the git:stash* arg aliases below the import block in filesystem.ts. Adds useSourceControlStash.test.ts covering the race, facade identity, the worktree-switch reset, picker dispatch, and the folder-workspace no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
162b343 to
60264b6
Compare
|
Second round — all four addressed in 60264b6, and one of them was a real product bug. The e2e comment on Two of my own tests should have caught it and didn't: the unit test exercised
e2e cleanup — done, with one adjustment. A blanket "discard every entry" Separately, I noticed a local Playwright run had wiped three tracked Re-verified: |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx (1)
279-309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStash count never refreshes when the Create-PR composer's dropdown is used instead of
CommitArea's.Both dropdowns render the same
dropdownItems(Stash entries included), but onlyCommitArea'sDropdownMenuis wired to triggerstash.refreshStashCount()on open (viaonDropdownOpenChange).CreateHostedReviewComposer'sDropdownMenuhas no such hook, and its props type has no slot for one. SinceSourceControl.tsxdocuments that the stash count is only ever read on menu-open (no polling), any timedirectCreatePrActionis truthy (a common state — whenever a hosted review can be created directly), users opening the composer's "More actions" menu will see the Stash submenu stuck showing "Checking stashes…"/disabled indefinitely.
src/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsx#L279-L309: add an optionalonDropdownOpenChange?: (open: boolean) => voidprop and wire it to<DropdownMenu onOpenChange={onDropdownOpenChange}>.src/renderer/src/components/right-sidebar/SourceControl.tsx#L5768-L5854: passonDropdownOpenChange={handleDropdownOpenChange}to<CreateHostedReviewComposer>alongside the existingdropdownItems/onDropdownActionprops.
🧹 Nitpick comments (2)
src/shared/git-stash-commands.ts (1)
147-182: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
applycan close the remaining verify→act race by targeting the oid directly.The fix for the
ref === nullskip is in place, butrestoreStashstill has a window betweenassertStashEntryUnmoved'srev-parseand the actualstash apply|pop -- <ref>where a concurrentstash pushcan shift indices — the positionalrefused for the real command may no longer be the entry that was just verified. Per git's own docs,apply(but notpop) accepts a raw commit oid in place ofstash@{N}, so onceexpectedCommitOidis verified,applycan target that oid directly and be immune to any index shift in between.pop/dropremain stuck with positional refs since git requires the reflog form for those.🔒 Proposed fix: use the verified oid directly for `apply`
await assertStashEntryUnmoved(exec, worktreePath, ref ?? 'stash@{0}', expectedCommitOid) try { await exec( - ref === null ? ['stash', subcommand] : ['stash', subcommand, '--', ref], + // Why: `apply` accepts a raw commit oid, so once verified this targets the + // exact entry regardless of any index shift between the check above and now. + subcommand === 'apply' && expectedCommitOid + ? ['stash', 'apply', '--', expectedCommitOid] + : ref === null + ? ['stash', subcommand] + : ['stash', subcommand, '--', ref], worktreePath )Please confirm this oid-acceptance behavior against the Git version(s) this app targets before adopting.
src/renderer/src/components/right-sidebar/source-control-stash-menu-items.ts (1)
228-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the already-computed busy/loading reasons.
resolveSubmenuTitlere-declares the same twotranslatecalls asbusyReason/loadingReason(Lines 31-38). Passing them in keeps a single definition per string so a copy change can't drift between the trigger and its rows.♻️ Proposed refactor
- title: resolveSubmenuTitle({ globalBusy, loading, stashes, hasAnyChanges }), + title: resolveSubmenuTitle({ globalBusy, loading, stashes, hasAnyChanges, busyReason, loadingReason }),function resolveSubmenuTitle(inputs: { globalBusy: boolean loading: boolean stashes: number hasAnyChanges: boolean + busyReason: string + loadingReason: string }): string { if (inputs.globalBusy) { - return translate( - 'auto.components.right.sidebar.source.control.stash.menu.items.busy', - 'Another git operation is in progress…' - ) + return inputs.busyReason } if (inputs.loading) { - return translate( - 'auto.components.right.sidebar.source.control.stash.menu.items.loading', - 'Checking stashes…' - ) + return inputs.loadingReason }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb70755f-94c0-466c-b5ff-9b1787fdcb1f
📒 Files selected for processing (43)
docs/reference/git-compatibility.mdsrc/main/git/stash-real-repo.test.tssrc/main/git/stash.test.tssrc/main/git/stash.tssrc/main/ipc/filesystem.tssrc/main/providers/git-stash-provider.tssrc/main/providers/ssh-git-provider.tssrc/main/providers/types.tssrc/main/runtime/orca-runtime-git.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/git-stash-params.tssrc/main/runtime/rpc/methods/git-stash.test.tssrc/main/runtime/rpc/methods/git.tssrc/main/runtime/runtime-rpc.tssrc/preload/api-types.tssrc/preload/index.tssrc/relay/git-handler-stash.test.tssrc/relay/git-handler.tssrc/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/SourceControlStashPicker.tsxsrc/renderer/src/components/right-sidebar/source-control-dropdown-entries.tsxsrc/renderer/src/components/right-sidebar/source-control-dropdown-items.test.tssrc/renderer/src/components/right-sidebar/source-control-dropdown-items.tssrc/renderer/src/components/right-sidebar/source-control-stash-actions.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.test.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.test.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/runtime/runtime-git-client.tssrc/renderer/src/web/web-preload-api.tssrc/shared/git-binary-compatibility.test.tssrc/shared/git-stash-commands.tssrc/shared/git-stash-conflict.tssrc/shared/git-stash-list-output.test.tssrc/shared/git-stash-list-output.tssrc/shared/git-stash-types.tstests/e2e/source-control-stash-actions.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/reference/git-compatibility.md
- src/renderer/src/i18n/locales/zh.json
- src/renderer/src/i18n/locales/es.json
| /** | ||
| * `ref` omitted targets the newest entry; `expectedCommitOid` is the oid the | ||
| * client saw when it listed, so a concurrent stash cannot silently retarget the | ||
| * operation at a different entry. | ||
| */ | ||
| export const GitStashRestore = WorktreeSelector.extend({ | ||
| ref: StashRef.optional(), | ||
| expectedCommitOid: ExpectedStashCommitOid.optional() | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require an OID when ref is omitted.
GitStashRestore currently accepts an unguarded request with neither ref nor expectedCommitOid. A concurrent stash can change the newest entry, causing restore/pop to target a different stash than the client selected.
Add a cross-field refinement requiring expectedCommitOid when ref is omitted, plus a regression test.
Proposed schema guard
export const GitStashRestore = WorktreeSelector.extend({
ref: StashRef.optional(),
expectedCommitOid: ExpectedStashCommitOid.optional()
+}).superRefine((value, ctx) => {
+ if (value.ref === undefined && value.expectedCommitOid === undefined) {
+ ctx.addIssue({
+ code: 'custom',
+ path: ['expectedCommitOid'],
+ message: 'expectedCommitOid is required when ref is omitted'
+ })
+ }
})Based on the PR objective to enforce OID checks when ref is omitted.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * `ref` omitted targets the newest entry; `expectedCommitOid` is the oid the | |
| * client saw when it listed, so a concurrent stash cannot silently retarget the | |
| * operation at a different entry. | |
| */ | |
| export const GitStashRestore = WorktreeSelector.extend({ | |
| ref: StashRef.optional(), | |
| expectedCommitOid: ExpectedStashCommitOid.optional() | |
| }) | |
| /** | |
| * `ref` omitted targets the newest entry; `expectedCommitOid` is the oid the | |
| * client saw when it listed, so a concurrent stash cannot silently retarget the | |
| * operation at a different entry. | |
| */ | |
| export const GitStashRestore = WorktreeSelector.extend({ | |
| ref: StashRef.optional(), | |
| expectedCommitOid: ExpectedStashCommitOid.optional() | |
| }).superRefine((value, ctx) => { | |
| if (value.ref === undefined && value.expectedCommitOid === undefined) { | |
| ctx.addIssue({ | |
| code: 'custom', | |
| path: ['expectedCommitOid'], | |
| message: 'expectedCommitOid is required when ref is omitted' | |
| }) | |
| } | |
| }) |
| stashList: async ({ worktreePath }) => { | ||
| const { entries } = await callRuntimeResult<{ entries: GitStashEntry[] }>( | ||
| 'git.stashList', | ||
| await stashParams(worktreePath) | ||
| ) | ||
| return entries | ||
| }, | ||
| stashPush: async ({ worktreePath, includeUntracked, message }) => | ||
| callRuntimeResult<GitStashPushResult>('git.stashPush', { | ||
| ...(await stashParams(worktreePath)), | ||
| includeUntracked: includeUntracked === true, | ||
| ...(message !== undefined ? { message } : {}) | ||
| }), | ||
| stashApply: async ({ worktreePath, ref, expectedCommitOid }) => | ||
| callRuntimeResult<GitStashMutationResult>('git.stashApply', { | ||
| ...(await stashParams(worktreePath)), | ||
| ...stashTargetParams(ref, expectedCommitOid) | ||
| }), | ||
| stashPop: async ({ worktreePath, ref, expectedCommitOid }) => | ||
| callRuntimeResult<GitStashMutationResult>('git.stashPop', { | ||
| ...(await stashParams(worktreePath)), | ||
| ...stashTargetParams(ref, expectedCommitOid) | ||
| }), | ||
| stashDrop: async ({ worktreePath, ref, expectedCommitOid }) => { | ||
| await callRuntimeResult('git.stashDrop', { | ||
| ...(await stashParams(worktreePath)), | ||
| ...stashTargetParams(ref, expectedCommitOid) | ||
| }) | ||
| }, | ||
| stashClear: async ({ worktreePath }) => { | ||
| await callRuntimeResult('git.stashClear', await stashParams(worktreePath)) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stash mutations may need a longer timeout on the web runtime path.
runtime-git-client.ts defines a dedicated 30s STASH_MUTATION_TIMEOUT_MS for stash push/apply/pop/drop/clear over SSH/runtime RPC specifically because stash push rewrites the whole working tree and can be slow. Here, the same operations (stashList/Push/Apply/Pop/Drop/Clear) call callRuntimeResult with no timeout override, unlike syncFork in this same file which explicitly passes 60_000. If the default timeout is shorter than the mutation budget used elsewhere, large-repo stash pushes could fail with a spurious timeout on the web client while succeeding via the desktop/SSH path.
🕒 Suggested fix
+const STASH_MUTATION_TIMEOUT_MS = 30_000
+
stashPush: async ({ worktreePath, includeUntracked, message }) =>
callRuntimeResult<GitStashPushResult>('git.stashPush', {
...(await stashParams(worktreePath)),
includeUntracked: includeUntracked === true,
...(message !== undefined ? { message } : {})
- }),
+ }, STASH_MUTATION_TIMEOUT_MS),(apply the same override to stashApply, stashPop, stashDrop, stashClear.)
#!/bin/bash
# Locate callRuntimeResult's default timeout to confirm the gap.
rg -n -A 10 'function callRuntimeResult' src/renderer/src/web/web-preload-api.ts
5f287d9 to
533983e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/right-sidebar/SourceControl.tsx (1)
5853-5853: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRefresh stash count when the PR composer dropdown opens.
CreateHostedReviewComposerpasses the samedropdownItemstoSourceControlDropdownEntries, but it doesn’t receive an open-change callback andstash.stashCountstays undefined. AddonDropdownOpenChangewiring / props support here, or refresh in the dropdown-render path, so existing-stash actions are not rendered disabled with the loading reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d165195f-2050-4d94-822f-74092eede942
📒 Files selected for processing (10)
src/main/git/stash-real-repo.test.tssrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/SourceControlStashMessageDialog.tsxsrc/renderer/src/components/right-sidebar/source-control-stash-actions.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.test.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.test.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.tssrc/renderer/src/i18n/locales/en.jsontests/e2e/source-control-stash-actions.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/components/right-sidebar/source-control-stash-menu-items.ts
- src/renderer/src/components/right-sidebar/source-control-stash-menu-items.test.ts
533983e to
b6fcd2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/relay/git-handler-stash.test.ts (1)
178-207: 🎯 Functional Correctness | 🔵 TrivialConsider adding a case for a present-but-malformed
ref/expectedCommitOid.Existing tests cover missing/well-formed and clearly-invalid string refs (
'--all','-p', etc.), but not a non-string value (e.g.ref: 123). That's exactly the gap flagged ingit-handler.ts'sreadStashRefParam/readExpectedStashOidParam— a regression test here would pin the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 987da85b-3c65-4fe8-85a1-fcd8e6796eae
📒 Files selected for processing (44)
docs/reference/git-compatibility.mdsrc/main/git/stash-real-repo.test.tssrc/main/git/stash.test.tssrc/main/git/stash.tssrc/main/ipc/filesystem.tssrc/main/providers/git-stash-provider.tssrc/main/providers/ssh-git-provider.tssrc/main/providers/types.tssrc/main/runtime/orca-runtime-git.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/git-stash-params.tssrc/main/runtime/rpc/methods/git-stash.test.tssrc/main/runtime/rpc/methods/git.tssrc/main/runtime/runtime-rpc.tssrc/preload/api-types.tssrc/preload/index.tssrc/relay/git-handler-stash.test.tssrc/relay/git-handler.tssrc/renderer/src/components/right-sidebar/CreateHostedReviewComposer.tsxsrc/renderer/src/components/right-sidebar/SourceControl.tsxsrc/renderer/src/components/right-sidebar/SourceControlStashMessageDialog.tsxsrc/renderer/src/components/right-sidebar/SourceControlStashPicker.tsxsrc/renderer/src/components/right-sidebar/source-control-dropdown-entries.tsxsrc/renderer/src/components/right-sidebar/source-control-dropdown-items.test.tssrc/renderer/src/components/right-sidebar/source-control-dropdown-items.tssrc/renderer/src/components/right-sidebar/source-control-stash-actions.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.test.tssrc/renderer/src/components/right-sidebar/source-control-stash-menu-items.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.test.tssrc/renderer/src/components/right-sidebar/useSourceControlStash.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/runtime/runtime-git-client.tssrc/renderer/src/web/web-preload-api.tssrc/shared/git-binary-compatibility.test.tssrc/shared/git-stash-commands.tssrc/shared/git-stash-conflict.tssrc/shared/git-stash-list-output.test.tssrc/shared/git-stash-list-output.tssrc/shared/git-stash-types.tstests/e2e/source-control-stash-actions.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/src/i18n/locales/es.json
- docs/reference/git-compatibility.md
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/i18n/locales/zh.json
|
Third round reviewed. Three fixed, two I'm pushing back on — all now squashed into the single commit b6fcd2b. Fixed
Not changing
Re-verified after the fixes: |
b6fcd2b to
1cb904c
Compare
|
Fourth round — two fixed in 1cb904c, and one of them was a good catch on the relay boundary. Param readers laundering malformed values — correct, and the sharper part of the finding is that my own docstring promised the opposite of what the code did. A present-but-non-string Locale ellipsis (the The Verified: I'm resolving the threads that are actually addressed. Two stay open deliberately, since they're judgement calls rather than defects and I'd rather a maintainer weigh in than have my disagreement buried:
Happy to implement either if you disagree with my reading. |
Orca had no stash support at any layer — the only way to stash was to type it
into a terminal. This adds eight actions in a nested Stash group at the bottom
of the commit split-button dropdown: Stash, Stash (Include Untracked), Pop and
Apply (latest or picked), Drop with a picker, and Drop All. Both stash rows
prompt for an optional name first, so entries are identifiable in the picker
instead of a wall of "WIP on main: ...".
Every row always renders, disabled with its own reason in the tooltip, matching
the invariant the sibling rows already follow. An untracked-only tree disables
plain Stash and points at Stash (Include Untracked) rather than letting git
report a confusing "No local changes to save" — `hasUnstagedChanges` is true for
untracked files too, so the submenu takes an explicit tracked-entry count.
The command core lives in src/shared/git-stash-commands with the git runner
injected, so local, WSL, SSH relay, and remote-runtime RPC all execute identical
argv and identical result interpretation instead of drifting copies.
Decisions worth knowing:
- No capability gate. Every flag predates the Git 2.25 baseline; `--staged`
(2.35) is the one exception, which is why a staged-only variant was left out.
Verified against a real 2.25.5 binary, asserted in the compatibility suite.
- Pop/apply conflicts mirror git: the entry is kept and the UI says so, naming
the surviving ref. Auto-dropping would destroy the user's only copy of a
half-applied changeset.
- Picked entries carry their commit oid. `stash@{N}` is positional and agents
run git in the same worktree, so a concurrent stash would otherwise silently
retarget a destructive action; a mismatch fails with stash_entry_moved before
anything runs.
- Naming distinguishes confirm-empty (git writes its own subject) from cancel
(aborts). Collapsing them would make Escape silently stash.
- The stash count is read on demand — menu open, panel visible, post-mutation —
not folded into the 60s status poll, where it would cost a subprocess per
worktree per minute for something only shown while the menu is open.
Stash refs are validated at three independent layers (RPC schema, shared command
core, and the relay, which is reachable independently of the schema).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1cb904c to
4c4383a
Compare
3 participants
Summary
Adds
git stashactions to the Source Control panel. Orca had zero stash support at any layer before this —grep -ri stash src/main/git src/preload src/renderer/src/runtimereturned only error strings and agent prompts telling agents not to run it. The only way to stash was to type it into a terminal.Eight actions live in a nested Stash group at the bottom of the commit split-button dropdown:
-u; prompts for an optional nameThe group follows the module's existing invariant — every row always renders, disabled with a reason in its tooltip, so the menu shape stays stable and testable. An untracked-only tree disables Stash with "Only untracked files — use Stash (Include Untracked)" rather than letting git report a confusing "no local changes to save".
Works on all four hosts: local, WSL, SSH relay, and remote-runtime RPC.
Three decisions worth reviewing
One implementation, not four. The command core lives in
src/shared/git-stash-commands.tswith the git runner injected, so main and the relay execute identical argv and identical result interpretation. This deliberately avoids the pattern incommitChangesRelay, which carries a "keep the two paths in sync" comment because it is a hand-maintained copy.No capability gate — and that is verified, not assumed. Every flag used predates the Git 2.25 baseline (
stash pushis 2.13;-m,--include-untracked,list --format/-z,apply|pop|drop|clear [--] <ref>are older).git stash push --stagedis 2.35, which is exactly why a staged-only variant was left out of the op set. I built Git 2.25.5 from source and ran the compatibility suite against it — a new case ingit-binary-compatibility.test.tsexercises the whole stash argv, so the claim is enforced by the existing real-binary CI matrix rather than by a version comment.Pop conflicts mirror git rather than compensating. On conflict git keeps the entry and says so. Auto-dropping would destroy the user's only copy of a half-applied changeset; auto-reverting would discard resolution work. The result carries
conflicted: true, the entry survives, and the toast names the survivingstash@{N}.Naming a stash
Both stash rows prompt for an optional name before running git, so entries are identifiable in the picker instead of a wall of
WIP on main: …. Confirming an empty field is a valid choice — the message is omitted and git writes its own subject — while cancelling aborts the stash entirely. Those two outcomes are deliberately distinct; collapsing them would make Escape silently stash.The
messageparameter was already built, validated, and tested through every transport in the first round of this PR, but nothing set it, so it was unreachable. This closes that.Index-shift guard
stash@{N}is positional and Orca runs agents against the same worktree, so an agent stashing between the picker rendering and the user clicking would silently retarget a destructive action at someone else's work. Picked entries carry their commit oid; a mismatch fails withstash_entry_movedbefore the destructive command runs. Covered by a real-repo test that reproduces the shift.Stash count is read on demand
Deliberately not added to
useGitStatusPolling. That hook runs for every worktree on a 60s interval with backoff added specifically because of poll churn (and has a regression spec). Upstream status rides along in porcelain v2 for free; a stash count is a separategit logonrefs/stashand only matters while the menu is open. It refreshes on menu open, on panel visibility, and after a mutation.Screenshots
before
after
Screen.Recording.2026-07-30.at.12.09.34.mp4
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildpnpm test: 40327 passed. 11 failures in 3 files (local-pty-shell-ready,pty-shell-launch,mobile-terminal-reveal-tab-adoption) — all PTY/terminal, none touching git. Verified identical failures onmain(3 files, 11 failed / 75 passed), so they are pre-existing and unrelated.New coverage:
git-stash-list-output.test.ts— NUL parsing, colon-bearing subjects,-z-ignored fallback, CRLF, truncated trailing record.stash.test.ts— exact argv per op, ref rejection, conflict classification,stashed:false, error ladder, index-shift guard.stash-real-repo.test.ts— real temp repos: full round trip,-usemantics, a real induced pop conflict proving the entry survives, index shift, detached HEAD, dash-prefixed message.git-handler-stash.test.ts— the relay/SSH path against a real repo, including method registration and re-validation.git-stash.test.ts— RPC schema rejection of bad refs/oids before they reach the runtime.source-control-stash-menu-items.test.ts— every row's enabled state and disabled reason, including "busy beats loading beats empty" and the untracked-only case below.useSourceControlStash.test.ts— the worktree-switch race, facade identity, and the naming prompt (typed name reaches git, empty omits the field, cancel does not stash).source-control-stash-actions.spec.ts(e2e) — the menu, an untracked-only tree, cancelling the name prompt, and a real stash → pop round trip asserted on the file row, not store state.E2E passes locally (4/4). Note
e2eis not a required check and only runs on PRs touchingtests/e2e/**— which this one does.AI Review Report
Self-review with Claude, focused on the risks this change actually carries.
Cross-platform (macOS / Linux / Windows). The stash argv contains no filesystem paths — there are no pathspecs, so
literalPathspecis not needed and no separator assumptions exist.wslDistrois threaded throughgitOptionsForWorktreeon every call, so native and WSL stay distinct. The list parser tolerates CRLF. No new keyboard shortcuts, accelerators, or shortcut labels, so nometaKey/CmdOrCtrlsurface. Menu rows are plain items with no platform-conditional copy.What the review flagged, and what changed as a result:
-mneeded guarding against a--prefixed message. Checking against real git showed-mconsumes the next argv token regardless, and args are passed as an array — the guard was never load-bearing. Rather than leave a misleading rationale, I corrected the comment to state the real reason (bound empty/huge subjects) and added a real-repo test that stores--allas a message verbatim, so the assumption is enforced rather than asserted.rev-parse --verify --quietexits 128, not 1, on a missing stash. My first implementation expected empty stdout and would have surfaced a raw git error instead ofstash_entry_moved. Found by probing real git; fixed and covered by a test.DropdownEntry.CreateHostedReviewComposerrenders the same array asCommitArea. Widening the union to add the submenu would have broken it at typecheck. Extracted a sharedsource-control-dropdown-entries.tsxrenderer so both surfaces migrate together — both files got shorter.(e): e is DropdownItem => e.kind !== 'separator'silently mistyped the submenu as a row (TS accepts a narrowing predicate to a subtype). Replaced with a real guard.gitStashCountByWorktree; worktree-keyed maps need purge wiring and there is a dedicatedworktree-removal-maps-leakspec guarding that class of bug. Since the count is consumed by exactly one component, I scoped it to component state instead — no purge path to forget.Also checked, no change needed:
max-lines(never disabled or bumped;providers/types.tswas at 291/300, which is precisely whyGitStashProvideris a separate file, and the dropdown test was at 785/800, hence a separate stash test file); folder workspaces (Source Control early-returns for non-git repos, and the hook no-ops onisGitWorkspace: false); GitLab/GitHub neutrality (stash is provider-agnostic, nothing added to hosted-review paths); detached HEAD (covered by test — stash works and reads(no branch)).Security Audit
Argument injection — the main risk, defended at three independent layers. A stash ref must match
/^stash@\{\d+\}$/: at the RPC zod schema, again in the shared command core, and the relay re-validates because its entrypoint is reachable independently of the schema (params arrive asRecord<string, unknown>). This blocks--prefixed flags,--all,-p, and arbitrary revs from reachingdrop/pop. Every invocation also passes--before the ref. Rejection is covered by tests at each layer.Command execution. No shell strings are constructed anywhere; all args go through
execFileas arrays. Stash messages are bounded to 500 chars and are safe as-mvalues (verified above).Path handling.
worktreePathis never trusted from the renderer on the local path —resolveRegisteredWorktreePathresolves it against the store, matchinggit:stage. No user-supplied paths enter the stash argv at all.IPC / transport surface. Six new
git:stash*channels and six RPC methods, each following the established SSH-provider-fork → resolve → route shape.src/relay/git-exec-validator.tsis deliberately untouched — its allow-list correctly rejectsstash, andgit-exec-validator.test.tsasserts that; the whole point of dedicated relay methods is to avoid loosening the read-onlygit.execpassthrough.Destructive-action safety. Drop and Drop All are gated behind
confirmVariant: 'destructive'confirmations, and the oid guard means a concurrent stash cannot silently redirect them. An older relay returns-32601, which is translated into an actionable reconnect hint rather than a raw JSON-RPC error.No new dependencies, secrets, auth paths, or network calls.
Review round
Findings from the automated review, all verified against the code before acting:
hasUnstagedChangesis true for untracked files too, so passing it through as "has tracked changes" left the row clickable with nothing to save —git stash pushexits 0 with "No local changes to save". The submenu now takes an explicit tracked-entry count. My unit test had missed it by exercising the resolver directly, and the e2e had encoded the wrong expectation because its helper created a new file despite being namedseedTrackedChange. Fixed, with a red-then-green test and a dedicated e2e case.await) does not work here, because the closure is the worktree it was created for.expectedCommitOidwas ignored when the ref was implicit, so "pop the latest, but only if it is still the one I saw" ran unguarded. Now verified againststash@{0}.useCallbacka no-op. Memoized, and the call sites now depend on the individual stable functions.Also restored three tracked
test-results/*.mdfiles that a local Playwright run had wiped; they were never part of this change.Notes
pnpm sync:localization-catalog; all five locales are in parity and both localization gates pass. The non-English catalogs carry the English strings until translated, matching how the tooling seeds new keys.git stash push --staged. It is the one stash flag above the 2.25 baseline, so it needs a realGitCapabilityslug plus fallback — deliberately out of scope here to keep this change capability-free.🤖 Generated with Claude Code