Repository navigation
feat(desktop): install GitHub CLI from onboarding terminal and add gh dialog end states - #6117
Conversation
|
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:
📝 WalkthroughWalkthroughThe desktop onboarding flow detects Homebrew and GitHub CLI authentication, supports GitHub CLI installation and authentication modes, streams terminal output, handles one-time codes, checks authentication status, and manages success, failure, retry, and dialog reset states. ChangesGitHub CLI onboarding
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant OnboardingPage
participant SystemRouter
participant GhAuthDialog
participant GhAuthTerminal
participant GitHubCLI
OnboardingPage->>SystemRouter: query Homebrew availability
SystemRouter-->>OnboardingPage: return installation status
OnboardingPage->>GhAuthDialog: open install or auth mode
GhAuthDialog->>GhAuthTerminal: provide mode-specific command
GhAuthTerminal->>GitHubCLI: start command
GitHubCLI-->>GhAuthTerminal: stream output and exit
GhAuthTerminal-->>GhAuthDialog: forward output and exit
GhAuthDialog->>GitHubCLI: check authentication status
GitHubCLI-->>GhAuthDialog: return authentication result
GhAuthDialog-->>OnboardingPage: close after success
🚥 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 |
End-to-end CDP verification (dev app, worktree renderer on vite port 8065, CDP 9612)Drove the real onboarding flow in the dev desktop app with End-to-end checks (real button clicks, real PTY, real brew + gh):
Synthetic-assisted check:
Screenshots in Cleanup done: shims reverted, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx`:
- Around line 191-195: Add the select-text and cursor-text classes to the error
message paragraph rendered in GhAuthDialog, preserving its existing text styling
and conditional message content.
- Line 112: Update the dialog className in GhAuthDialog so the 752px maximum
width is applied with the sm breakpoint variant, ensuring it overrides the
existing sm:max-w-lg rule while preserving the current gap styling.
In `@apps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsx`:
- Around line 30-35: Update the onboarding installation flow around detectBrew
and the Install action to wait until the brewStatus query has resolved before
choosing an installation path. Disable the Install action or show its loading
state while detection is pending, and only use the external fallback after
detection explicitly reports Homebrew unavailable; preserve the existing
Homebrew path once availability is confirmed.
🪄 Autofix (Beta)
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: f3483a12-a659-46c2-84d9-be2c380758fd
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/system.tsapps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/index.tsapps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsx
| const { | ||
| data: ghStatus, | ||
| refetch: refetchGh, | ||
| isFetching: isFetchingGh, | ||
| } = electronTrpc.system.detectGhCli.useQuery(); | ||
| const { data: brewStatus } = electronTrpc.system.detectBrew.useQuery(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wait for Homebrew detection before selecting the installation path.
Until detectBrew resolves, brewAvailable is false. A user can click Install during that interval and open the external website even when Homebrew is installed. Disable the Install action or show a loading state until the query resolves. Use the external fallback only after detection reports Homebrew unavailable.
Also applies to: 53-80
🤖 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/renderer/routes/_authenticated/onboarding/page.tsx` around
lines 30 - 35, Update the onboarding installation flow around detectBrew and the
Install action to wait until the brewStatus query has resolved before choosing
an installation path. Disable the Install action or show its loading state while
detection is pending, and only use the external fallback after detection
explicitly reports Homebrew unavailable; preserve the existing Homebrew path
once availability is confirmed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsx:53">
P2: `brewAvailable` is derived straight from `brewStatus?.installed`, which is `false` while the `detectBrew` query is still loading. If a user clicks Install before the query resolves, `openGitHubInstall` will open the external cli.github.com link even when Homebrew is actually installed, instead of launching the in-app install terminal. Consider disabling the action or showing a loading state until `detectBrew` settles.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx:78">
P2: Reopening during a previous status check can have the old check mark the new auth attempt successful and auto-close it. Guard this completion with a per-open/per-attempt token (or cancel it) before updating phase or scheduling the close.</violation>
</file>
Architecture diagram
sequenceDiagram
participant UI as Onboarding Page
participant Dialog as GhAuthDialog
participant Terminal as GhAuthTerminal
participant PTY as PTY Process
participant TRPC as tRPC System Router
participant Shell as Shell Environment
Note over UI,Shell: GitHub CLI Setup Flow
UI->>UI: Detect brew status
UI->>TRPC: detectBrew.query()
TRPC->>Shell: execWithShellEnv("brew", ["--version"])
Shell-->>TRPC: stdout + version
TRPC-->>UI: { installed: boolean }
alt Brew available
UI->>UI: Set dialog mode = "install"
UI->>Dialog: Open with GH_INSTALL_COMMAND
else Brew not available
UI->>UI: window.open("https://cli.github.com/")
end
Note over Dialog,PTY: Dialog Lifecycle
Dialog->>Dialog: Reset state (phase="running", attempt++)
Dialog->>Terminal: Create with key={attempt}
Terminal->>PTY: spawn (command)
PTY-->>Terminal: onData (chunks)
Terminal->>Dialog: onOutput(data)
Note over Dialog,Terminal: One-time code detection
Dialog->>Dialog: Buffer output, strip ANSI
Dialog->>Dialog: Match /one-time code: ([A-Z0-9]{4}-[A-Z0-9]{4})/
alt Code found
Dialog->>UI: Render code with copy button
UI->>UI: navigator.clipboard.writeText(code)
end
Note over Dialog,PTY: Process exit
PTY-->>Terminal: onExit(exitCode)
Terminal->>Dialog: handleTerminalExit()
Dialog->>Dialog: Set phase = "checking"
Dialog->>TRPC: detectGhCli.query()
TRPC->>Shell: gh --version + gh auth status
Shell-->>TRPC: status
TRPC-->>Dialog: { installed, authenticated }
alt Success (installed && authenticated)
Dialog->>Dialog: Set phase = "success"
Dialog->>Dialog: Auto-close after 1.5s
Dialog->>UI: onExit()
UI->>TRPC: refetchGh()
else Failure
Dialog->>Dialog: Set phase = "failed"
alt Retry clicked
Dialog->>Dialog: handleRetry()
Dialog->>Terminal: New terminal (key=attempt+1)
end
end
Note over Dialog,UI: Outside-click protection
alt Process active (running/checking)
UI->>UI: onInteractOutside.preventDefault()
UI->>UI: onFocusOutside.preventDefault()
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| !!anthropicStatus?.authenticated && !anthropicStatus.issue; | ||
| const codexConnected = !!openAIStatus?.authenticated && !openAIStatus.issue; | ||
|
|
||
| const brewAvailable = brewStatus?.installed === true; |
There was a problem hiding this comment.
P2: brewAvailable is derived straight from brewStatus?.installed, which is false while the detectBrew query is still loading. If a user clicks Install before the query resolves, openGitHubInstall will open the external cli.github.com link even when Homebrew is actually installed, instead of launching the in-app install terminal. Consider disabling the action or showing a loading state until detectBrew settles.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsx, line 53:
<comment>`brewAvailable` is derived straight from `brewStatus?.installed`, which is `false` while the `detectBrew` query is still loading. If a user clicks Install before the query resolves, `openGitHubInstall` will open the external cli.github.com link even when Homebrew is actually installed, instead of launching the in-app install terminal. Consider disabling the action or showing a loading state until `detectBrew` settles.</comment>
<file context>
@@ -47,7 +50,13 @@ function OnboardingDashboardPage() {
!!anthropicStatus?.authenticated && !anthropicStatus.issue;
const codexConnected = !!openAIStatus?.authenticated && !openAIStatus.issue;
+ const brewAvailable = brewStatus?.installed === true;
+
const openGitHubInstall = () => {
</file context>
| onExitRef.current(); | ||
| void electronTrpcClient.system.detectGhCli | ||
| .query() | ||
| .then((status) => { |
There was a problem hiding this comment.
P2: Reopening during a previous status check can have the old check mark the new auth attempt successful and auto-close it. Guard this completion with a per-open/per-attempt token (or cancel it) before updating phase or scheduling the close.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx, line 78:
<comment>Reopening during a previous status check can have the old check mark the new auth attempt successful and auto-close it. Guard this completion with a per-open/per-attempt token (or cancel it) before updating phase or scheduling the close.</comment>
<file context>
@@ -1,38 +1,203 @@
+ onExitRef.current();
+ void electronTrpcClient.system.detectGhCli
+ .query()
+ .then((status) => {
+ if (status.installed && status.authenticated) {
+ setPhase("success");
</file context>
… dialog end states - system.detectBrew procedure (Homebrew detection via login-shell PATH) - Install action opens the terminal dialog running brew install gh + gh auth login when brew is present; external link fallback otherwise - GhAuthDialog end states: re-run detectGhCli on PTY exit, brief success state with 1.5s auto-close, failure message with Retry (fresh paneId) - one-time device code chip with copy button, parsed from stripped output - block outside-click dismissal while the process is running (non-modal Dialog default would kill the PTY mid-auth) - description copy matches gh's actual prompt sequence
…strip One morphing strip under the title replaces description paragraph, code card, and loose state rows: prompts hint -> auto-copied one-time code -> red failure with inline Retry. Fix dialog width (sm:max-w-lg was beating max-w-[752px]), dim terminal once the code appears, refocus terminal after manual Copy so Enter reaches gh.
121511e to
a99b464
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx`:
- Around line 131-139: Update the DialogContent configuration in GhAuthDialog to
add an onEscapeKeyDown handler that prevents the default dismissal when
processActive is true, matching the existing onInteractOutside and
onFocusOutside guards.
🪄 Autofix (Beta)
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: f6703f02-cd60-41e0-8a80-5db976286812
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/system.tsapps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/index.tsapps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/index.ts
- apps/desktop/src/lib/trpc/routers/system.ts
- apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsx
- apps/desktop/src/renderer/routes/_authenticated/onboarding/page.tsx
| <DialogContent | ||
| className="gap-3 sm:max-w-[752px]" | ||
| aria-describedby={undefined} | ||
| onInteractOutside={(event) => { | ||
| if (processActive) event.preventDefault(); | ||
| }} | ||
| onFocusOutside={(event) => { | ||
| if (processActive) event.preventDefault(); | ||
| }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
path="apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx"
if [ ! -f "$path" ]; then
echo "Missing file: $path"
git ls-files | rg 'GhAuthDialog\.tsx|GhAuthDialog' || true
exit 0
fi
echo "== file outline =="
ast-grep outline "$path" --view expanded || true
echo "== relevant source =="
wc -l "$path"
sed -n '1,180p' "$path" | cat -n
echo "== all dialog dismissal handler references in file =="
rg -n "onEscapeKeyDown|onInteractOutside|onFocusOutside|processActive|setPhase|detectGhCli|GhAuthTerminal|handleTerminalExit" "$path"Repository: superset-sh/superset
Length of output: 8389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect Dialog implementation =="
git ls-files | rg '`@superset/ui/dialog`|dialog' | sed -n '1,80p' || true
if [ -f "apps/desktop/src/lib/ui/index.ts" ]; then
echo "== ui index excerpt =="
sed -n '1,220p' apps/desktop/src/lib/ui/index.ts | cat -n
fi
if [ -d "apps/desktop/src/lib/ui" ]; then
echo "== ui dialog files =="
git ls-files "apps/desktop/src/lib/ui" | rg -i 'dialog|ui|components' || true
fi
echo "== package imports maybe linked via source =="
rg -n 'DialogContent|useDialog|AlertDialog|Radix.*Dialog|`@radix-ui/react-dialog`' apps/desktop/src || true
echo "== inspect local package metadata if present =="
for f in package.json bun.lockb bun.lock pnpm-lock.yaml yarn.lock; do
[ -f "$f" ] && echo "-- $f --" && (rg -n '"`@radix-ui/react-dialog`"|`@radix-ui/react-dialog`|DialogContent' "$f" || true)
doneRepository: superset-sh/superset
Length of output: 50377
🌐 Web query:
Radix UI Dialog Content onEscapeKeyDown default behavior Escape key props example
💡 Result:
The default behavior of the Radix UI Dialog Content component is to close the dialog and return focus to the trigger element when the Escape key is pressed [1][2][3]. To customize or prevent this behavior, you can use the onEscapeKeyDown prop on the Dialog.Content component [1][2]. By calling event.preventDefault within the handler, you can stop the dialog from closing [4][5][6]. Example of preventing the Escape key from closing the dialog: <Dialog.Content onEscapeKeyDown={(event) => { event.preventDefault; }} > {/* Dialog content */} </Dialog.Content> You can also use this prop to execute custom logic before or instead of the default close action, such as showing a confirmation prompt or checking conditions before allowing the dialog to close [5][6].
Citations:
- 1: https://www.radix-ui.com/primitives/docs/components/dialog
- 2: https://radix-ui-primitives.mintlify.app/components/dialog
- 3: https://reactlibs.dev/articles/radix-dialog-modal-mastery/
- 4: How to prevent close alert dialog with esc? radix-ui/primitives#1430
- 5: How to prevent close alert dialog with esc? radix-ui/primitives#1434
- 6: https://vercel.com/academy/shadcn-ui/anatomy-of-a-primitive
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local dialog component =="
sed -n '1,220p' packages/ui/src/components/ui/dialog.tsx | cat -n
echo "== local DialogContent exports/usages =="
rg -n "export const DialogContent|DialogContent|DialogContentContent|onEscapeKeyDown|onInteractOutside|onFocusOutside" packages/ui/src/components/ui/dialog.tsx packages/ui/src/components/ui/alert-dialog.tsx || true
echo "== GhAuthTerminal source =="
file="apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsx"
if [ -f "$file" ]; then
wc -l "$file"
ast-grep outline "$file" --view expanded || true
sed -n '1,240p' "$file" | cat -n
fiRepository: superset-sh/superset
Length of output: 10050
Guard Escape dismissal while a shell process is active.
GhAuthTerminal mounts a terminal and calls terminal.kill.mutate on unmount, and handleTerminalExit starts detectGhCli.query(). Since DialogContent accepts dialog dismissal props, add onEscapeKeyDown that calls event.preventDefault() when processActive is true, matching the outside-escape handlers.
🔒 Proposed fix to guard the Escape key
onFocusOutside={(event) => {
if (processActive) event.preventDefault();
}}
+ onEscapeKeyDown={(event) => {
+ if (processActive) event.preventDefault();
+ }}📝 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.
| <DialogContent | |
| className="gap-3 sm:max-w-[752px]" | |
| aria-describedby={undefined} | |
| onInteractOutside={(event) => { | |
| if (processActive) event.preventDefault(); | |
| }} | |
| onFocusOutside={(event) => { | |
| if (processActive) event.preventDefault(); | |
| }} | |
| <DialogContent | |
| className="gap-3 sm:max-w-[752px]" | |
| aria-describedby={undefined} | |
| onInteractOutside={(event) => { | |
| if (processActive) event.preventDefault(); | |
| }} | |
| onFocusOutside={(event) => { | |
| if (processActive) event.preventDefault(); | |
| }} | |
| onEscapeKeyDown={(event) => { | |
| if (processActive) event.preventDefault(); | |
| }} |
🤖 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/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx`
around lines 131 - 139, Update the DialogContent configuration in GhAuthDialog
to add an onEscapeKeyDown handler that prevents the default dismissal when
processActive is true, matching the existing onInteractOutside and
onFocusOutside guards.
- run the dialog modal so the overlay dims the onboarding page behind it - queue renderer->pty input until createOrAttach resolves: xterm's automatic query replies (OSC 11, DSR) could hit a not-yet-created pane, and the terminal service synthesizes an exit for dead-pane writes, flipping the dialog to the failure state while the command was starting - stash a pre-ready exit and replay it after create resolves so instant commands still resolve end states
Redesign + rebase updateRebased onto main after #6115 merged — page.tsx resolved as a merge with main's UI winning: polling/ Dialog redesigned into a single guided status strip (per design review): title + one morphing strip + terminal, nothing else. Strip states: spinner "Follow the prompts below" → one-time code (auto-copied to clipboard on arrival, Now modal: the dialog renders the fading overlay so the onboarding page behind it is dimmed — outside clicks are still hard-blocked while the process runs. Race found and fixed during CDP verification: xterm auto-replies to terminal queries (OSC 11 / DSR) immediately at mount; if such a write reached the pane before Re-verified end-to-end via CDP (sandboxed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsx`:
- Around line 96-112: Update the createOrAttach promise flow in GhAuthTerminal
to track whether the effect has been disposed and whether the component is still
mounted. On creation failure, call fireExit() only while mounted so GhAuthDialog
can enter its retry state; when creation resolves after disposal, kill the newly
created pane instead of marking it ready or writing pending input, and retain
normal cleanup behavior otherwise.
🪄 Autofix (Beta)
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: 63e95086-689a-4379-b9e2-03ae901cdeec
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsxapps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthDialog.tsx
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… escape hatch gh auth status --active only exists on gh >= 2.40 (2023-12); retry without the flag so older installs don't read as permanently signed out. Install-mode failure strip gains an Install manually link to cli.github.com as the exit from the embedded flow.
…anguage Design audit pass: failure keeps the neutral strip surface with red only on the icon, spinner strips drop to muted regular weight, success loses the tinted circle badge, failure copy shortened to "didn't complete".
…g in gh dialog Spinner strips read as subtitles (bg only, no border) per review. If createOrAttach rejects, fire the exit path so the dialog lands on the failure strip instead of running forever with a dead terminal.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsx:115">
P3: The new `.catch(() => fireExit())` closes the dialog on a failed terminal create but silently discards the rejection reason, so a create failure (e.g. a bad command path or PTY spawn error) is unobservable and hard to diagnose. Consider logging the error with context before firing exit, e.g. `console.warn("gh auth terminal create failed", err)`, so repro attempts and support issues have a signal to trace.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }) | ||
| // A failed create would otherwise leave the dialog running forever | ||
| // with a dead terminal silently swallowing queued input. | ||
| .catch(() => fireExit()); |
There was a problem hiding this comment.
P3: The new .catch(() => fireExit()) closes the dialog on a failed terminal create but silently discards the rejection reason, so a create failure (e.g. a bad command path or PTY spawn error) is unobservable and hard to diagnose. Consider logging the error with context before firing exit, e.g. console.warn("gh auth terminal create failed", err), so repro attempts and support issues have a signal to trace.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/onboarding/components/GhAuthDialog/GhAuthTerminal.tsx, line 115:
<comment>The new `.catch(() => fireExit())` closes the dialog on a failed terminal create but silently discards the rejection reason, so a create failure (e.g. a bad command path or PTY spawn error) is unobservable and hard to diagnose. Consider logging the error with context before firing exit, e.g. `console.warn("gh auth terminal create failed", err)`, so repro attempts and support issues have a signal to trace.</comment>
<file context>
@@ -109,7 +109,10 @@ export function GhAuthTerminal({
+ })
+ // A failed create would otherwise leave the dialog running forever
+ // with a dead terminal silently swallowing queued input.
+ .catch(() => fireExit());
return () => {
</file context>
| .catch(() => fireExit()); | |
| .catch((err) => { | |
| console.warn("gh auth terminal create failed", err); | |
| fireExit(); | |
| }); |
…captured gh waits for Enter only so the user can copy the code first; the dialog already auto-copies it, so send Enter to the pty on detection and open the browser immediately. Terminal keeps full opacity at the code step (dim now only marks the dead failure state); hint copy updated to the single remaining action.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Part of SUPER-1729 — https://linear.app/superset-sh/issue/SUPER-1729
What
system.detectBrew: new tRPC procedure detecting Homebrew viaexecWithShellEnv("brew", ["--version"])on the login-shell PATH (explicitBrewDetectResultinterface).brew install gh && gh auth login --hostname github.com --git-protocol https --web(title "Install GitHub CLI"). When brew is absent, it keeps the existing external link to https://cli.github.com/.detectGhCli. Installed + authenticated → brief success state, auto-close after 1.5s (row still refetches viaonExit). Otherwise the dialog stays open with "Sign-in canceled or failed." and a Retry button that respawns a fresh terminal (new paneId via remount). The stream's exit payload does carryexitCode, but success is decided solely fromdetectGhClisince the auth outcome is what matters.strip-ansi, already a desktop dep), and matched against/one-time code: ([A-Z0-9]{4}-[A-Z0-9]{4})/; the code renders above the terminal with a copy button. Regex verified against gh's actual output shape (! First copy your one-time code: XXXX-XXXXwith the code bold-wrapped, across chunk boundaries).modal={false}, so a stray outside click unmounted the terminal and killed the PTY mid-auth. While the process is running/checking,onInteractOutside/onFocusOutsideare prevented.? Authenticate Git with your GitHub credentials? (Y/n)).Deliberately unchanged: Continue is still not gated on gh state (PR #4834); page.tsx diff kept minimal since row status rendering is being reworked on another branch.
Verification
bun run lint:fix+bun run lint→ exit 0apps/desktopbun run typecheck→ cleanSummary by CodeRabbit
Screenshot evidence (CDP-verified)
Before (SUPER-1729 walkthrough): device code buried in a bare terminal with no copy affordance; Ctrl+C left a dead
^Cwith no feedback —After:
Images live on the
evidence/super-1729branch (never merge; deletable after the PRs land).Screenshots (current UI, CDP-verified in the dev app)
Guided single-strip layout: title + one morphing status strip + terminal.
Running — follow the prompts (install mode, full 752px width)
Code ready — auto-copied, terminal dims (Copy fallback shown when the clipboard write is rejected, e.g. window unfocused)
Failure — strip turns red, Retry inline (respawns a fresh pane)
Modal overlay — background dimmed while the flow runs
Success — auto-closes after ~1.5s, row refetches to Connected
