Repository navigation
fix: make worktree scan failures actionable - #21291
Conversation
937acd9 to
2658bec
Compare
|
Follow-up accuracy review addressed before merge:
Focused validation now passes 7 tests and oxlint. |
2658bec to
706e266
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds structured worktree scan failure kinds for Xcode license, developer tools, architecture mismatch, and unknown errors. Main-process metadata fallbacks now include the classified kind. Renderer validation and result comparisons account for Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Some unrelated Git startup failures may receive incorrect architecture-mismatch guidance. This does not block scanning or recovery, but the classifier should be narrowed before merge if accurate remediation is required. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the change, motivation, testing, platform scope, and user-facing behavior. However, the required Linked Issue field is incomplete because it contains only Full details: Docstring CoverageExplanation Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 8 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
979cf63 to
eeaadbe
Compare
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The new copy actions are unreachable. They render inside TooltipContent, which the shared primitive pins to pointer-events-none and whose provider defaults disableHoverableContent = true ("app tooltips are non-interactive labels"), so neither mouse nor keyboard can activate them. This is the PR's central affordance, so it currently ships non-functional.
Reviewed changes
- Shared classifier —
classifyWorktreeScanFailuremaps raw Git stderr toxcode-license/developer-tools/architecture-mismatch/unknown, returning a plain-language message and a fix command for the two Apple cases. - Tooltip content — the worktree scan warning now shows the classified message plus
fixCommand, with "Copy command" / "Copy diagnostics" buttons copying throughnavigator.clipboard. - Classifier tests — three cases covering the two Apple classifications and the architecture spawn error.
ℹ️ Nitpicks
- Classifier tests pin the three known branches but not the
unknownfallback (that it echoes the raw reason) or the fact thatarchitecture-mismatchintentionally carries nofixCommand; those are the contract the PR description calls out. - The component test (
RepoScanUnavailableIndicator.test.tsx) still only asserts the trigger button, so the copy actions have no coverage — which is why the interactivity issue wasn't caught. navigator.platform(line 40) is deprecated; the repo standard isnavigator.userAgent.includes('Mac').
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| const copyText = async (value: string): Promise<void> => { | ||
| await navigator.clipboard?.writeText(value) |
There was a problem hiding this comment.
navigator.clipboard?.writeText is the one clipboard path the codebase documents as unreliable in Orca's renderer, and the optional chaining plus missing catch make a failed copy indistinguishable from a successful one. Every other copy affordance routes through window.api.ui.writeClipboardText.
Technical details
# Copy uses a clipboard path that fails silently in the renderer
## Affected sites
- `src/renderer/src/components/sidebar/worktree-list/rows/RepoScanUnavailableIndicator.tsx:43-44` — `await navigator.clipboard?.writeText(value)`; caller uses `void copyText(...)`, so a rejection is unhandled.
- Precedent documenting the failure: `src/renderer/src/components/native-chat/NativeChatCopyButton.tsx:6-12`, `src/renderer/src/components/sidebar/AutoRenameFailedDialog.tsx:77-80`, `src/renderer/src/components/quick-open-install-rg-guidance.tsx:85-88`.
## Required outcome
- Copies go through `window.api.ui.writeClipboardText` and surface success/failure.
- A rejected write must not become an unhandled rejection, and a missing clipboard must not silently no-op.
## Suggested approach
- Reuse `useClipboardTextCopyFeedback` (`src/renderer/src/hooks/use-clipboard-text-copy-feedback.ts`): it wraps the IPC, returns a `copied`/`failed` status, and guards state after unmount.| Copy command | ||
| </button> | ||
| ) : null} | ||
| <button | ||
| type="button" | ||
| className="text-xs underline" | ||
| onClick={() => void copyText(diagnosticText)} | ||
| > | ||
| Copy diagnostics | ||
| </button> |
There was a problem hiding this comment.
The two new labels are raw literals while every other string in this component goes through translate(...); the classifier message values are likewise English-only and rendered directly. The localization gates pass only because they verify referenced keys, not untranslated literals.
Technical details
# New user-facing strings bypass the i18n catalog
## Affected sites
- `RepoScanUnavailableIndicator.tsx:97,105` — "Copy command" / "Copy diagnostics" literals.
- `RepoScanUnavailableIndicator.tsx:38-41` — diagnostics labels ("Repository:", "Path:", …).
- `src/shared/worktree-scan-failure.ts:11,20,27-28` — `message` values shown in the tooltip.
## Required outcome
- User-visible copy resolves through the localization catalog like the rest of the component.
## Suggested approach
- Wrap the labels in `translate(...)` and run `pnpm run sync:localization-catalog`.
- The shared classifier cannot import renderer i18n: return `kind` and map it to localized messages in the component, keeping the raw reason for `unknown`.
8c41a73 to
1736da8
Compare
|
Addressed the valid review findings:
The requested focused tests now pass 5/5, and oxlint plus React lint pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 65028f5d-4d7a-4683-a908-455004c04ff4
📒 Files selected for processing (8)
src/main/ipc/worktrees/listing/detected-provider-listing.tssrc/renderer/src/components/sidebar/worktree-list/rows/RepoScanUnavailableIndicator.tsxsrc/renderer/src/store/slices/worktrees/listing/detected-worktree-host-merge.tssrc/renderer/src/store/slices/worktrees/listing/detected-worktree-provider-request.tssrc/renderer/src/store/slices/worktrees/listing/worktree-catalog-visibility.tssrc/shared/worktree-scan-failure.test.tssrc/shared/worktree-scan-failure.tssrc/shared/worktree/types.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
| if (/no developer tools were found/i.test(reason)) { | ||
| return 'developer-tools' | ||
| } | ||
| if (/Unknown system error -86|EBADARCH|Bad CPU type in executable/i.test(reason)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,100p' src/shared/worktree-scan-failure.ts
sed -n '1,160p' src/shared/worktree-scan-failure.test.ts
rg -n -- 'Unknown system error -86|EBADARCH|Bad CPU type|architecture-mismatch' src test* 2>/dev/null || true
node - <<'NODE'
const current = /Unknown system error -86|EBADARCH|Bad CPU type in executable/i
const proposed = /\bUnknown system error -86\b|\bEBADARCH\b|Bad CPU type in executable/i
for (const s of ['Unknown system error -86', 'Unknown system error -860', 'Unknown system error -861']) {
console.log(s, { current: current.test(s), proposed: proposed.test(s) })
}
NODERepository: stablyai/orca
Length of output: 2696
🏁 Script executed:
rg -n -C 4 --glob '*.ts' --glob '*.tsx' --glob '*.json' 'classifyWorktreeScanFailure|worktree-scan-failure|architecture-mismatch|Unknown system error -86|EBADARCH|Bad CPU type|scan failure|scanFailure' src package.jsonRepository: stablyai/orca
Length of output: 22468
🏁 Script executed:
sed -n '130,175p' src/main/ipc/worktrees/listing/detected-provider-listing.ts
sed -n '48,75p' src/renderer/src/components/sidebar/worktree-list/rows/RepoScanUnavailableIndicator.tsx
rg -n -C 5 --glob '*.ts' --glob '*.tsx' 'function describeWorktreeScanFailure|describeWorktreeScanFailure|unavailableReason' src/main/ipc/worktrees/listing src/shared src/renderer/src/store/slices/worktreesRepository: stablyai/orca
Length of output: 21522
🏁 Script executed:
sed -n '1,55p' src/main/ipc/worktrees/listing/worktree-listing-diagnostics.tsRepository: stablyai/orca
Length of output: 1428
Delimit the numeric error code.
The current expression matches Unknown system error -860 because -86 is an unbounded substring. The listing stores that result as architecture-mismatch, and the local Mac UI can then show incorrect CPU-architecture guidance. Use a digit lookahead instead of a word boundary, because the numeric code starts after a hyphen. Add a near-match test for -860.
Proposed fix
- if (/Unknown system error -86|EBADARCH|Bad CPU type in executable/i.test(reason)) {
+ if (/Unknown system error -86(?!\d)|\bEBADARCH\b|Bad CPU type in executable/i.test(reason)) {📝 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.
| if (/Unknown system error -86|EBADARCH|Bad CPU type in executable/i.test(reason)) { | |
| if (/Unknown system error -86(?!\d)|\bEBADARCH\b|Bad CPU type in executable/i.test(reason)) { |
There was a problem hiding this comment.
Caution
Two blocking problems remain at this head: the five new translate() keys are absent from en.json, so verify:localization-catalog and verify:localization-extraction both fail, and the copy actions are still rendered inside a non-interactive TooltipContent and still copy through navigator.clipboard.
Reviewed changes (since the prior review at 937acd9)
- Classifier returns a kind only —
classifyWorktreeScanFailurenow returnsWorktreeScanFailureKind; the main process classifiesdescribeWorktreeScanFailure(err)and shipsfailureKindonDetectedWorktreeListResult. - Renderer maps kind to localized copy — the indicator selects the message and fix command from a table, gated to local-Mac repos, and builds a host-aware diagnostics string.
- Store plumbing — merge and catalog-equality comparisons include
failureKind; the provider-request validator accepts only known kinds. - Tests — classifier cases expanded to four (adding
invalid active developer path→unknown); new strings wrapped intranslate().
ℹ️ invalid active developer path now gets no fix command
The developer-tools matcher was narrowed to /no developer tools were found/, and the new test pins invalid active developer path to unknown. That path error is the common post-macOS-upgrade Git breakage, and it is the case where xcode-select --install (or --reset) is the standard repair — so this may undercut the PR's goal for the most frequent macOS failure.
Technical details
# Narrowed developer-tools matcher drops the common macOS repair
## Affected sites
- `src/shared/worktree-scan-failure.ts:11` — `/no developer tools were found/i` is now the only developer-tools signal.
- `src/shared/worktree-scan-failure.test.ts:15-19` — asserts `invalid active developer path` → `unknown`.
## Required outcome
- Decide whether `invalid active developer path` / `xcode-select: error:` should keep offering `xcode-select --install`; if not, the tooltip for that case is raw stderr with no next step.
## Open questions for the human
- Is the intent to avoid prescribing `--install` when the developer dir is merely misconfigured (e.g. a custom Xcode path), or was this a side effect of tightening `xcode-select: error:`?ℹ️ Nitpicks
detectedWorktreeFailureKinds(detected-worktree-provider-request.ts:20-25) manually mirrors theWorktreeScanFailureKindunion and can drift when a kind is added; exporting aconsttuple fromworktree-scan-failure.tsand deriving both would keep them in sync.fixCommandForFailureKind(component lines 23-31) is more verbose than aPartial<Record<WorktreeScanFailureKind, string>>lookup overWORKTREE_SCAN_FIX_COMMANDS.navigator.platform(lines 54, 77) is deprecated; the repo standard isnavigator.userAgent.includes('Mac').
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| <div className="flex items-center justify-start gap-3 border-t border-border/60 pt-1"> | ||
| {fixCommand ? ( | ||
| <button | ||
| type="button" | ||
| className="text-xs underline" | ||
| onClick={() => void copyText(fixCommand)} | ||
| > | ||
| {translate( | ||
| 'auto.components.sidebar.RepoScanUnavailableIndicator.copyCommand', | ||
| 'Copy command' | ||
| )} | ||
| </button> | ||
| ) : null} | ||
| <button | ||
| type="button" | ||
| className="text-xs underline" | ||
| onClick={() => void copyText(diagnosticText)} | ||
| > | ||
| {translate( | ||
| 'auto.components.sidebar.RepoScanUnavailableIndicator.copyDiagnostics', | ||
| 'Copy diagnostics' | ||
| )} | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Carried from the prior review and still present: these buttons sit inside TooltipContent, which the shared primitive pins to pointer-events-none and whose provider defaults disableHoverableContent = true, so pointer events pass through and the tooltip closes once the pointer leaves the trigger — neither mouse nor keyboard can activate them.
Technical details
# Interactive controls inside a non-interactive tooltip
## Affected sites
- `src/renderer/src/components/sidebar/worktree-list/rows/RepoScanUnavailableIndicator.tsx:128-151` — the footer buttons render inside `TooltipContent` with no `pointer-events-auto`.
- `src/renderer/src/components/ui/tooltip.tsx:46` — content className includes `pointer-events-none`; provider (line 8) defaults `disableHoverableContent = true`, commented "app tooltips are non-interactive labels".
- `src/renderer/src/App.tsx:92` — app root mounts the default provider.
## Required outcome
- The copy actions must be reachable by mouse and keyboard without holding hover on the trigger.
- Per `docs/STYLEGUIDE.md` (Tooltips), interactive content and blocking errors do not belong in a tooltip.
## Suggested approach
- Move the message + actions into a click-revealed `Popover` (or an inline row under the repo) and keep the tooltip as a short label on the icon button.
- If a tooltip must carry them, add `pointer-events-auto` to the content and set `disableHoverableContent={false}` for this instance — but that fights the primitive's documented contract.| const copyText = async (value: string): Promise<void> => { | ||
| await navigator.clipboard?.writeText(value) |
There was a problem hiding this comment.
Carried from the prior review and still present: navigator.clipboard is the path this repo documents as failing silently in the renderer and inside Radix surfaces, and the optional chaining plus missing catch make a failed copy indistinguishable from a successful one. Every other copy affordance routes through window.api.ui.writeClipboardText.
Technical details
# Copy uses a clipboard path that fails silently in the renderer
## Affected sites
- `src/renderer/src/components/sidebar/worktree-list/rows/RepoScanUnavailableIndicator.tsx:81-82` — `await navigator.clipboard?.writeText(value)`; callers use `void copyText(...)`, so a rejection is unhandled.
- Precedent documenting the failure: `NativeChatCopyButton.tsx:6-12`, `AutoRenameFailedDialog.tsx:77-80`, `quick-open-install-rg-guidance.tsx:85-88`.
## Required outcome
- Copies go through `window.api.ui.writeClipboardText` and surface success/failure.
- A rejected write must not become an unhandled rejection, and a missing clipboard must not silently no-op.
## Suggested approach
- Reuse `useClipboardTextCopyFeedback` (`src/renderer/src/hooks/use-clipboard-text-copy-feedback.ts`), which wraps the IPC, exposes a copied/failed status, and guards state after unmount.
1736da8 to
b0ccb25
Compare
There was a problem hiding this comment.
Important
The interactivity fix works, but three items remain: the five new translate() keys are still absent from en.json (both localization gates fail on this head), the copy still uses navigator.clipboard (the path this repo documents as failing silently in the renderer), and there is still no success/failure feedback on copy.
Reviewed changes (delta from 8c41a73)
- Tooltip copy actions are now reachable — the trigger is wrapped in a scoped
<TooltipProvider disableHoverableContent={false}>and the content carriespointer-events-auto, so the buttons receive pointer events instead of falling through. - Platform detection de-deprecated —
navigator.platform→navigator.userAgent.includes('Mac'), and the diagnostics line now emitsClient platform: macOS. - Classifier test — added the unknown-fallback case.
ℹ️ Nitpicks
- The nested
<TooltipProvider>omitsdelayDuration, so the wrapper's default (0) replaces the app's 400 ms hover delay for this tooltip; passdelayDuration={400}if the instant-open behavior is unintended. detectedWorktreeFailureKinds(detected-worktree-provider-request.ts:20-25) still manually mirrorsWorktreeScanFailureKindand can drift; a sharedconsttuple used to derive both would keep them in sync.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| const copyText = async (value: string): Promise<void> => { | ||
| await navigator.clipboard?.writeText(value) |
There was a problem hiding this comment.
Still outstanding from the previous two reviews: navigator.clipboard is the path this repo documents as failing silently in the renderer and inside Radix surfaces, and the optional chaining plus missing catch make a failed copy indistinguishable from a successful one — there is also no copied/failed feedback. Every other copy affordance routes through window.api.ui.writeClipboardText.
Technical details
# Copy uses a clipboard path that fails silently in the renderer
## Affected sites
- `src/renderer/src/components/sidebar/worktree-list/rows/RepoScanUnavailableIndicator.tsx:81-82` — `await navigator.clipboard?.writeText(value)`; callers use `void copyText(...)`, so a rejection is unhandled.
- Precedent documenting the failure: `NativeChatCopyButton.tsx:6-12`, `AutoRenameFailedDialog.tsx:77-80`, `quick-open-install-rg-guidance.tsx:85-88`.
## Required outcome
- Copies go through `window.api.ui.writeClipboardText` and surface success/failure.
- A rejected write must not become an unhandled rejection, and a missing clipboard must not silently no-op.
## Suggested approach
- Reuse `useClipboardTextCopyFeedback` (`src/renderer/src/hooks/use-clipboard-text-copy-feedback.ts`), which wraps the IPC, exposes a copied/failed status, and guards state after unmount.
b0ccb25 to
d9ffc6b
Compare
There was a problem hiding this comment.
Important
The catalog fix is in — verify:localization-catalog and verify:localization-extraction now pass. One item remains: the copy actions still write through navigator.clipboard, which this repo documents as failing silently in the renderer, and a failed copy is currently indistinguishable from a successful one.
Reviewed changes (delta from 1736da8)
- English catalog — added
xcodeLicense,developerTools,architectureMismatch,copyCommand, andcopyDiagnosticsunder theRepoScanUnavailableIndicatornamespace; both localization gates pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| const copyText = async (value: string): Promise<void> => { | ||
| await navigator.clipboard?.writeText(value) |
There was a problem hiding this comment.
Still outstanding: navigator.clipboard is the path this repo documents as failing silently in the renderer and inside Radix surfaces, and the optional chaining plus missing catch make a failed copy indistinguishable from a successful one — there is no copied/failed feedback. Every other copy affordance routes through window.api.ui.writeClipboardText.
Technical details
# Copy uses a clipboard path that fails silently in the renderer
## Affected sites
- `src/renderer/src/components/sidebar/worktree-list/rows/RepoScanUnavailableIndicator.tsx:81-82` — `await navigator.clipboard?.writeText(value)`; callers use `void copyText(...)`, so a rejection is unhandled.
- Precedent documenting the failure: `NativeChatCopyButton.tsx:6-12`, `AutoRenameFailedDialog.tsx:77-80`, `quick-open-install-rg-guidance.tsx:85-88`.
## Required outcome
- Copies go through `window.api.ui.writeClipboardText` and surface success/failure.
- A rejected write must not become an unhandled rejection, and a missing clipboard must not silently no-op.
## Suggested approach
- Reuse `useClipboardTextCopyFeedback` (`src/renderer/src/hooks/use-clipboard-text-copy-feedback.ts`), which wraps the IPC, exposes a copied/failed status, and guards state after unmount.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline. The copy now goes through
window.api.ui.writeClipboardText, and the previous blockers (tooltip interactivity, missing catalog keys) are resolved.
Reviewed changes (delta from b0ccb25)
- Clipboard routing —
copyTextnow callswindow.api.ui.writeClipboardText(value)instead ofnavigator.clipboard?.writeText, matching every other copy affordance in the app.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| const copyText = async (value: string): Promise<void> => { | ||
| await window.api.ui.writeClipboardText(value) | ||
| } |
There was a problem hiding this comment.
Now that the write is routed correctly, consider whether a failed copy should stay invisible: onClick={() => void copyText(...)} leaves a rejected writeClipboardText unhandled, and neither button shows copied/failed state. useClipboardTextCopyFeedback (src/renderer/src/hooks/use-clipboard-text-copy-feedback.ts) handles both and is what the other copy buttons use.
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the change addresses the prior nitpicks. The earlier copy-feedback suggestion is the only item still open, so this is not an approval.
Reviewed changes (delta from d9ffc6b)
- Remote diagnostics preserved —
failureKindis now read fromdetected.failureKindon every host, so a remote host's classified cause reaches the tooltip instead of being forced tounknown. - Shared kind tuple —
WORKTREE_SCAN_FAILURE_KINDSplusisWorktreeScanFailureKindreplace the renderer-local Set that mirrored the union, so the validator cannot drift. - Fix-command lookup simplified —
WORKTREE_SCAN_FIX_COMMANDSisas const satisfies Partial<Record<WorktreeScanFailureKind, string>>andfixCommandForFailureKindindexes it directly. - Tooltip delay restored — the scoped provider's tooltip now passes
delayDuration={400}, matching the app default.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
* fix: make worktree scan failures actionable * fix: preserve remote worktree scan diagnostics
ELI5
When Git cannot scan a repository, the sidebar now explains the likely host problem and gives a safe next step instead of showing only the raw error.
What Changed
Why
The same host-level Git failure can affect every repository, but the old tooltip exposed only raw stderr and required users to discover the repair themselves. The classifier keeps known guidance specific and avoids prescribing a misleading architecture fix when the host evidence is incomplete.
Linked Issue
Fixes #
Visual Proof
After screenshot (fresh background Electron renderer; tooltip footer actions are left-aligned):
Testing
Focused renderer and classifier tests: 6 passed.
oxlint passed for changed files.
Manually validated the tooltip in a background Orca Electron instance through CDP on macOS.
SSH/Windows behavior is unchanged; classification is based only on returned Git diagnostics.
I manually tested these changes locally
Automated tests added/updated
Review
Please review whether the classifier wording matches the desired support guidance before broadening it to structured executable-path diagnostics.
Agent skill upstream boundary
Notes
The implementation does not automatically select an alternate Git executable. It reports actionable guidance while preserving existing PATH selection behavior.