Repository navigation
fix(desktop): repair tray updater require, guard clipboard writes, classify updater/keychain/job noise - #6190
Merged
Conversation
…assify updater/keychain/job noise Four Sentry desktop signals, all traced to a call site rather than the Sentry boundary (no beforeSend filters, no middleware allowlists): - The tray's "Check for Updates" item lazily `require`d "../auto-updater", a path that does not exist in the flat dist/main bundle, so the menu item threw MODULE_NOT_FOUND on every click. Now a static import on the same `main/lib/auto-updater` alias the other call sites use. - xterm's ClipboardAddon shipped the default BrowserClipboardProvider, whose writeText rejection is never handled. OSC 52 arrives from the PTY whether or not the window is focused, and Chromium refuses clipboard writes from an unfocused document. Both terminal stacks now pass a provider that skips the write when the document is unfocused and swallows NotAllowedError. - electron-updater starts the auto-download inside checkForUpdates and returns that promise unattached, so every Squirrel failure landed as an unhandled rejection: read-only volume, ENOENT renaming the cached pending zip, ENOENT lstat'ing ShipIt, request timeouts. Both check paths now consume the download promise; the existing `error` handler stays the single place that logs and drives status, and reports to Sentry only when the failure is not one of the known user-environment shapes. - A failing pre-push hook made git push reject with the hook's own output as the message, which escaped changes.push as INTERNAL_SERVER_ERROR and filed the user's build log as a Sentry issue. The push is now translated to PRECONDITION_FAILED with git's text preserved for the toast. Claude-Session: https://claude.ai/code/session_012u6VZtfQgoTh8qS4NqCb7e
Contributor
📝 WalkthroughWalkthroughThe desktop changes add Git push error mapping, improve updater failure handling and tray wiring, and add focus-aware clipboard support to terminal clipboard addons. ChangesDesktop behavior updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Tray
participant AutoUpdater
participant ElectronUpdater
participant Sentry
Tray->>AutoUpdater: checkForUpdatesInteractive()
AutoUpdater->>ElectronUpdater: checkForUpdates()
ElectronUpdater-->>AutoUpdater: UpdateCheckResult
AutoUpdater->>AutoUpdater: releaseDownloadPromise()
AutoUpdater->>Sentry: report non-environment updater errors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Greptile SummaryThe PR repairs the tray updater import, safely handles terminal clipboard writes, consumes updater download rejections, classifies known updater environment failures, and translates push errors into user-facing precondition failures.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking concern that the push handler now hides unexpected preparation and integration failures as user preconditions. The functional fixes are coherent, but the new push catch spans substantially more than git push rejection and prevents unexpected failures in that region from reaching Sentry. Files Needing Attention: apps/desktop/src/lib/trpc/routers/changes/git-operations.ts
|
| Filename | Overview |
|---|---|
| apps/desktop/src/lib/trpc/routers/changes/git-operations.ts | Adds useful push-failure classification, but the catch also suppresses unexpected failures from preparation helpers. |
| apps/desktop/src/main/lib/auto-updater.ts | Consumes updater download rejections and selectively captures unrecognized updater errors. |
| apps/desktop/src/main/lib/tray/index.ts | Replaces an invalid runtime require with a build-resolved static import; the resulting cycle has no unsafe evaluation-time access. |
| apps/desktop/src/renderer/lib/terminal/clipboard-provider.ts | Adds focus-aware clipboard writes while preserving propagation of unexpected errors. |
| apps/desktop/src/renderer/lib/terminal/terminal-addons.ts | Wires the shared clipboard provider into the v2 terminal stack. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts | Wires the same clipboard behavior into the legacy terminal stack. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Push[changes.push] --> Helpers[Resolve upstream / PR / remotes]
Helpers --> GitPush[Execute git push]
Helpers -->|non-TRPC error| Catch[New broad catch]
GitPush -->|non-TRPC error| Catch
Catch --> Precondition[PRECONDITION_FAILED]
Precondition --> User[Push failed toast]
Precondition -. skipped .-> Sentry[Sentry middleware]
Prompt To Fix All With AI
### Issue 1
apps/desktop/src/lib/trpc/routers/changes/git-operations.ts:102-115
**Catch spans push preparation**
The new catch translates every non-`TRPCError` from upstream resolution, GitHub PR lookup, remote discovery, and push execution into `PRECONDITION_FAILED`. This hides unexpected integration or application defects from Sentry and presents them as user-actionable push failures.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(desktop): repair tray updater requir..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+102
to
115
| }); | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof TRPCError) { | ||
| throw error; | ||
| } | ||
| // git refuses a push for reasons that live in the user's repo, | ||
| // remote or hooks — a rejected ref, missing credentials, a | ||
| // pre-push hook exiting non-zero. Surface git's own text. | ||
| throw new TRPCError({ | ||
| code: "PRECONDITION_FAILED", | ||
| message: error instanceof Error ? error.message : String(error), | ||
| cause: error, | ||
| }); |
Contributor
There was a problem hiding this comment.
The new catch translates every non-TRPCError from upstream resolution, GitHub PR lookup, remote discovery, and push execution into PRECONDITION_FAILED. This hides unexpected integration or application defects from Sentry and presents them as user-actionable push failures.
Knowledge Base Used: Desktop App (Electron main + renderer)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/lib/trpc/routers/changes/git-operations.ts
Line: 102-115
Comment:
**Catch spans push preparation**
The new catch translates every non-`TRPCError` from upstream resolution, GitHub PR lookup, remote discovery, and push execution into `PRECONDITION_FAILED`. This hides unexpected integration or application defects from Sentry and presents them as user-actionable push failures.
**Knowledge Base Used:** [Desktop App (Electron main + renderer)](https://app.greptile.com/personal-org-6810/-/custom-context/knowledge-base/superset-sh/superset/-/docs/desktop-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/changes/git-operations.ts (1)
90-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the error contract.
Test that a Git or hook
ErrorproducesPRECONDITION_FAILED, preserves the original message, and stores the originalcause. Also test that an existingTRPCErroris rethrown unchanged. Place these cases inapps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.ts` around lines 90 - 114, Add regression cases in git-operations.test.ts covering the push error handling around pushWithResolvedUpstream and pushCurrentBranch: verify ordinary Git or hook Errors become PRECONDITION_FAILED with the original message and cause preserved, and verify an existing TRPCError is rethrown by identity without wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.ts`:
- Around line 104-114: Update the error mapping around pushWithResolvedUpstream
and pushCurrentBranch so PR-target resolution, fetch, remote-resolution, and
other application failures map to INTERNAL_SERVER_ERROR, while Git push
rejection errors retain PRECONDITION_FAILED only when they represent repo-local
Git or hook failures. Add targeted tests asserting the mapped TRPCError.code for
each case.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.ts`:
- Around line 90-114: Add regression cases in git-operations.test.ts covering
the push error handling around pushWithResolvedUpstream and pushCurrentBranch:
verify ordinary Git or hook Errors become PRECONDITION_FAILED with the original
message and cause preserved, and verify an existing TRPCError is rethrown by
identity without wrapping.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c292bb2-7068-4f4a-9181-e3f09f1e5b98
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/main/lib/auto-updater.tsapps/desktop/src/main/lib/tray/index.tsapps/desktop/src/renderer/lib/terminal/clipboard-provider.tsapps/desktop/src/renderer/lib/terminal/terminal-addons.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
Comment on lines
+104
to
+114
| } catch (error) { | ||
| if (error instanceof TRPCError) { | ||
| throw error; | ||
| } | ||
| // git refuses a push for reasons that live in the user's repo, | ||
| // remote or hooks — a rejected ref, missing credentials, a | ||
| // pre-push hook exiting non-zero. Surface git's own text. | ||
| throw new TRPCError({ | ||
| code: "PRECONDITION_FAILED", | ||
| message: error instanceof Error ? error.message : String(error), | ||
| cause: error, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline apps/desktop/src/lib/trpc/routers/changes/utils/git-push.ts --items all
rg -n -C 10 \
'hasUpstreamBranch|getLocalBranchOrThrow|resolveExistingPullRequestPushTarget|resolveMismatchedPullRequestPushTarget|pushWithSetUpstream' \
apps/desktop/src/lib/trpc/routers/changes
rg -n -C 12 \
'PRECONDITION_FAILED|TRPCError|push' \
apps/desktop/src/lib/trpc/routers/changes/git-operations.test.tsRepository: superset-sh/superset
Length of output: 33053
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "git-operations.ts relevant section:"
sed -n '70,125p' apps/desktop/src/lib/trpc/routers/changes/git-operations.ts
echo
echo "git-push.ts relevant functions:"
sed -n '36,75p' apps/desktop/src/lib/trpc/routers/changes/utils/git-push.ts
sed -n '95,122p' apps/desktop/src/lib/trpc/routers/changes/utils/git-push.ts
sed -n '132,200p' apps/desktop/src/lib/trpc/routers/changes/utils/git-push.ts
sed -n '203,290p' apps.desktop/src/lib/trpc/routers/changes/utils/git-push.ts 2>/dev/null || sed -n '203,290p' apps/desktop/src/lib/trpc/routers/changes/utils/git-push.ts
echo
echo "git-utils exports:"
sed -n '1,160p' apps/desktop/src/lib/trpc/routers/changes/git-utils.ts
echo
echo "test coverage around error code mappings/reflection:"
rg -n -C 8 'PRECONDITION_FAILED|pushCurrentBranch|pushWithResolvedUpstream|non-TRPCError|catch' apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts apps/desktop/src/lib/trpc/routers/changes/utils -g '*.test.ts' || trueRepository: superset-sh/superset
Length of output: 7978
Align the mapped error with the operation that failed.
pushWithResolvedUpstream rethrows every non-TRPCError from PR-target resolution and pushCurrentBranch rethrows every non-TRPCError from git.push(), but apps/desktop/src/lib/trpc/routers/changes/git-operations.ts maps both kinds to PRECONDITION_FAILED. Use INTERNAL_SERVER_ERROR for fetch/remote-resolution/application failures, or add helpers that classify only Git hooks and repo-local Git errors. Add targeted tests for the mapped TRPCError.code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.ts` around lines 104
- 114, Update the error mapping around pushWithResolvedUpstream and
pushCurrentBranch so PR-target resolution, fetch, remote-resolution, and other
application failures map to INTERNAL_SERVER_ERROR, while Git push rejection
errors retain PRECONDITION_FAILED only when they represent repo-local Git or
hook failures. Add targeted tests asserting the mapped TRPCError.code for each
case.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 6 files
Architecture diagram
sequenceDiagram
participant Tray as Tray Menu
participant AutoUpdater as Auto Updater Lib
participant ElectronUpdater as electron-updater
participant Sentry as Sentry SDK
participant Process as Node Process
participant Terminal as Terminal Renderer
participant ClipboardAddon as Clipboard Addon
participant FocusAwareClipboardProvider as FocusAwareClipboardProvider
participant Navigator as Navigator API
participant GitRouter as Git Operations Router
participant Git as simple-git
Note over Tray,Git: Four distinct signal fixes
Tray->>AutoUpdater: click "Check for Updates"
AutoUpdater->>ElectronUpdater: checkForUpdates()
ElectronUpdater-->>AutoUpdater: UpdateCheckResult
AutoUpdater->>AutoUpdater: releaseDownloadPromise()
AutoUpdater->>AutoUpdater: consume .downloadPromise.catch(()=>{})
Note over AutoUpdater: Prevents unhandled rejection in main process
loop AutoUpdater.on("error")
ElectronUpdater->>AutoUpdater: error event
AutoUpdater->>AutoUpdater: isEnvironmentUpdateError()
alt Environment error (read-only, timeout, errno on updater path)
AutoUpdater->>AutoUpdater: log + clear cache + emit ERROR status
Note over AutoUpdater: No Sentry report
else Unrecognized error
AutoUpdater->>Sentry: captureException(error)
Note over AutoUpdater: Real bug still reaches Sentry
end
end
Note over Terminal,Navigator: Clipboard write in unfocused window
Terminal->>ClipboardAddon: writeText()
ClipboardAddon->>FocusAwareClipboardProvider: writeText()
FocusAwareClipboardProvider->>FocusAwareClipboardProvider: document.hasFocus()?
alt No focus
FocusAwareClipboardProvider->>ClipboardAddon: return silently
Note over FocusAwareClipboardProvider: Skip write, no error
else Has focus
FocusAwareClipboardProvider->>Navigator: navigator.clipboard.writeText()
alt NotAllowedError
Navigator-->>FocusAwareClipboardProvider: DOMException
FocusAwareClipboardProvider->>ClipboardAddon: swallow error
else Other error
FocusAwareClipboardProvider->>ClipboardAddon: rethrow
end
end
Note over GitRouter,Git: Push failure from pre-push hook
GitRouter->>Git: pushCurrentBranch()
alt TRPCError thrown
Git-->>GitRouter: TRPCError (already classified)
GitRouter->>GitRouter: throw unchanged
else Other error (e.g., GitError)
Git-->>GitRouter: Error
GitRouter->>GitRouter: new TRPCError(PRECONDITION_FAILED, git's message)
Note over GitRouter: Prevents raw GitError escaping as INTERNAL_SERVER_ERROR
end
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four Sentry desktop signals, each fixed at its call site. No
beforeSendfilters and no middleware allowlists were added — per the contract from #6164, anything still reaching Sentry is a real bug.1. Tray "Check for Updates" threw MODULE_NOT_FOUND (real bug)
apps/desktop/src/main/lib/tray/index.tslazilyrequired"../auto-updater"with a comment about breaking a circular dependency. That relative path does not exist in the flatdist/mainbundle, so every click on the menu item threwError: Cannot find module '../auto-updater'frommenu.click. Replaced with a static import onmain/lib/auto-updater, the same aliaslib/trpc/routers/auto-update/index.tsalready uses. The cycle it worried about (auto-updater↔main/index) already exists and is safe here becausecheckForUpdatesInteractiveis only called from a click handler, never at module evaluation.2. Unhandled clipboard rejection from OSC 52 (real bug)
Both terminal stacks constructed
new ClipboardAddon(new Utf8Base64()), leaving the addon's defaultBrowserClipboardProviderin place. That provider returnsnavigator.clipboard.writeText(...)and the addon never attaches a rejection handler. OSC 52 arrives from the PTY regardless of window focus, and Chromium refuses clipboard writes from an unfocused document, so background copies surfaced asNotAllowedError: Document is not focused.New
FocusAwareClipboardProvider(renderer/lib/terminal/clipboard-provider.ts) skips the write whendocument.hasFocus()is false and swallowsNotAllowedError, rethrowing anything else. Wired intorenderer/lib/terminal/terminal-addons.ts(v2) and the v1Terminal/helpers.ts.3. electron-updater noise (expected environment)
Root cause:
autoUpdater.checkForUpdates()starts the auto-download internally and hands backresult.downloadPromiseunattached. Nothing ever consumed it, so every Squirrel failure landed as an unhandled rejection in the main process — which is why these arrived withmechanism: auto.node.onunhandledrejectionand no stack trace, and why the existingautoUpdater.on("error")handler never suppressed them. That accounts forCannot update while running on a read-only volume(1083 events), theENOENT rename '…-updater/pending/….zip'andENOENT lstat '…ShipIt…'families, andThe request timed out.Both check paths now consume
downloadPromise. Theerrorevent stays the single classification point: it still logs, still clears the corrupt cache, and still emits the sameERRORstatus to the renderer, but now reports to Sentry only when the message is not a known user-environment shape (read-only volume, request timeout, or an errno on an updater cache path). Updater failures we do not recognize still reach Sentry — this is not a blanket swallow.4. Pre-push hook output filed as a Sentry issue (expected environment)
changes.pushlet simple-git'sGitErrorescape asINTERNAL_SERVER_ERROR, so a user's failing pre-push hook uploaded its entire build log as the Sentry error message (recordJobRun(jobs.ts)in the culprit line is the user's own project, not ours). The push is now translated toPRECONDITION_FAILEDwith git's own text preserved, so thePush failed: …toast is unchanged. Already-classifiedTRPCErrors thrown inside the push helpers pass through untouched.Not fixed
OSStatus -60006(DESKTOP-H5, authorization cancelled, message localized to Japanese): skipped. The event is an unhandled rejection in the main process with no stack trace, and nosafeStorage/keytar/Authorization Services call site exists undersrc/main. Identifying it would be speculation.changes.syncreaches the samepushCurrentBranchhelper and can still 500 on a failing pre-push hook. Left out to keep this diff scoped to the reported signal; worth a follow-up.Verification
bunx biome checkclean on all six files.apps/desktoptypecheck shows 67 errors, identical to the count on the base commit with the changes stashed — all from unbuilt@superset/chat-legacy/@superset/emailworkspace packages, none in the changed files.Claude-Session: https://claude.ai/code/session_012u6VZtfQgoTh8qS4NqCb7e
Summary by cubic
Fixes four desktop issues that caused noisy Sentry reports and a broken “Check for Updates” menu item. Improves updater error handling, guards background clipboard writes, and surfaces clearer git push errors.
main/lib/auto-updaterto stop MODULE_NOT_FOUND in the packaged app.FocusAwareClipboardProviderfor@xterm/addon-clipboardwithUtf8Base64; skips writes when unfocused and swallowsNotAllowedError.electron-updater, consumecheckForUpdates()downloadPromiseto prevent unhandled rejections; keep a singleerrorhandler and report to Sentry only for non-environment errors (not read-only volume, timeouts, or errno on-updater/ShipIt paths).TRPCErrorpush failures (e.g., pre-push hooks) toPRECONDITION_FAILEDwith git’s message; existingTRPCErrors pass through.Written for commit fcc9927. Summary will update on new commits.
Summary by CodeRabbit