feat(desktop): remove skip-all onboarding affordance#4113
Merged
Conversation
Per-step skipping is enough — drop the chrome-level "Skip onboarding" button and the associated `skipAll` store action. Each step's "Skip for now" link is unchanged.
Contributor
|
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:
📝 WalkthroughWalkthroughRemoves the global onboarding "skip all" capability, adds per-step "Skip for now" actions in several setup pages, refactors providers setup from a Tabs flow to independent ProviderSection components with per-provider state, and adds an OpenAI OAuth loopback utility plus chat-service and router integration to capture OAuth redirects locally. ChangesOnboarding Skip Removal & Per-step Skip UI
Providers Setup Refactor
OpenAI OAuth Loopback & Chat Service Integration
useOpenAIOAuth Hook: UI callback & completion changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User Browser
participant ChatService as ChatService (desktop)
participant Loopback as OpenAIOAuthLoopback (local server)
participant OpenAI as OpenAI Auth
ChatService->>OpenAI: initiate OAuth (returns auth URL with redirect_uri)
Browser->>OpenAI: user authenticates
OpenAI-->>Browser: redirect to loopback redirect_uri (http://localhost:PORT/path?code=...)
Browser->>Loopback: GET /path?code=...
Loopback->>ChatService: onCallback(callbackUrl)
ChatService->>OpenAI: exchange code for token (complete OAuth)
ChatService->>Loopback: stop()
Loopback-->>Browser: render success page
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 SummaryThis PR removes the chrome-level "Skip onboarding" button from
Confidence Score: 5/5Safe to merge — the change is a clean removal of a single UI affordance and its backing store action, with no side effects on the remaining onboarding paths. Both files contain straightforward deletions. The No files require special attention. The empty
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/setup/components/OnboardingProgress/OnboardingProgress.tsx | Removes the skipAll store subscription, handleSkipAll handler, and "Skip onboarding" button; leaves an empty div as a grid spacer for the third column. |
| apps/desktop/src/renderer/stores/onboarding/onboardingStore.ts | Drops the skipAll action and its skipped_all analytics event from the store interface and implementation; markSkipped/markComplete paths are unchanged. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User enters onboarding] --> B[providers step]
B -->|markComplete / markSkipped| C[gh-cli step]
C -->|markComplete / markSkipped| D[permissions step]
D -->|markComplete / markSkipped| E[project step]
E -->|markComplete / markSkipped| F[adopt-worktrees step]
F -->|finishFlow| G["track onboarding_finished\n(outcome: completed)"]
F -->|skipFlow| H["track onboarding_finished\n(outcome: skipped)"]
G --> I[Navigate to /welcome or project]
H --> I
style J fill:#f88,stroke:#c00
J["REMOVED: Skip onboarding button\n(skipAll → skipped_all event)"]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/renderer/routes/_authenticated/setup/components/OnboardingProgress/OnboardingProgress.tsx:103
**Empty layout spacer div**
The right-hand column is now empty after the button removal. Its only job is to balance the `grid-cols-[1fr_auto_1fr]` grid so the step pills remain visually centered. A brief comment would make this intentional spacer obvious to future readers; alternatively, the Tailwind classes on the div (`flex items-center justify-end`) can be dropped since there is no longer any content to align.
Reviews (1): Last reviewed commit: "feat(desktop): remove skip-all onboardin..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Skip onboarding | ||
| </button> | ||
| </div> | ||
| <div className="flex items-center justify-end" /> |
Contributor
There was a problem hiding this comment.
The right-hand column is now empty after the button removal. Its only job is to balance the grid-cols-[1fr_auto_1fr] grid so the step pills remain visually centered. A brief comment would make this intentional spacer obvious to future readers; alternatively, the Tailwind classes on the div (flex items-center justify-end) can be dropped since there is no longer any content to align.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/setup/components/OnboardingProgress/OnboardingProgress.tsx
Line: 103
Comment:
**Empty layout spacer div**
The right-hand column is now empty after the button removal. Its only job is to balance the `grid-cols-[1fr_auto_1fr]` grid so the step pills remain visually centered. A brief comment would make this intentional spacer obvious to future readers; alternatively, the Tailwind classes on the div (`flex items-center justify-end`) can be dropped since there is no longer any content to align.
How can I resolve this? If you propose a fix, please make it concise.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!
- Show Claude Code and Codex on the same providers page (no more tab toggle); each has its own selectable connection methods and Connect button, stacked vertically. - Ensure every step renders both Continue (primary) and "Skip for now" (link) in a stable bottom column. Adds the missing skip on the gh-cli installed branch and the adopt-worktrees empty branch.
![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
🤖 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/setup/providers/page.tsx`:
- Around line 215-234: ProviderSectionProps declares onReconfigure but
ProviderSection doesn't use it; update ProviderSection to accept and forward
that callback: add onReconfigure to the destructured params in the
ProviderSection function and inject it into the connectedPanel by checking
React.isValidElement(connectedPanel) and returning
React.cloneElement(connectedPanel, { onReconfigure }) so the ConnectedPanel
instance receives the handler; ensure React is imported if not already so
cloneElement/isValidElement work. This consolidates the reconfigure entry point
at ProviderSection without changing the call sites that already pass
onReconfigure.
🪄 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
Run ID: d92e04f1-3ba1-429f-b218-97aa82c415e6
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/setup/adopt-worktrees/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/gh-cli/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx
- Add a localhost loopback HTTP server that binds to the redirect_uri port (1455) advertised in the OpenAI authorize URL. When the browser hits /auth/callback, the server passes the URL straight to completeOpenAIOAuth so the user no longer has to copy/paste it. Falls back silently to the existing manual paste flow if the port can't be bound. - Tidy the providers page reconfigure UX: move Reconfigure/Cancel to a small inline link in the section header so the in-flow stack is just option cards + a single primary button instead of three stacked buttons.
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
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)
apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx (1)
39-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
useRefpreventsshouldAutoAdvancefrom ever becomingtrue— auto-advance silently broken.
wasConfiguredOnMountis auseRef, so mutating.currentinside the effect (line 42) does not schedule a re-render. The execution order is:
- Queries resolve → React re-renders; at render time
wasConfiguredOnMount.currentis stillnull→shouldAutoAdvance = false.- The capturing effect runs and sets
.current = atLeastOneConnected(potentiallytrue).- Nothing triggers another render — this component only subscribes to
completed.providers,manualWalkthrough,markComplete,markSkipped, andgoTo(all stable function refs).goTo("providers")writescurrentStepto the store, which no selector here watches.- The auto-advance effect sees
shouldAutoAdvance = falseand never navigates.The result is that returning users who already have a provider connected get stuck on this page until they click Continue manually, even though the intent is to auto-skip them.
Use
useStateso the "captured on mount" value triggers the required re-render:🐛 Proposed fix: replace `useRef` with `useState`
-import { type ReactNode, useEffect, useRef, useState } from "react"; +import { type ReactNode, useEffect, useState } from "react";- const wasConfiguredOnMount = useRef<boolean | null>(null); - useEffect(() => { - if (!isStatusPending && wasConfiguredOnMount.current === null) { - wasConfiguredOnMount.current = atLeastOneConnected; - } - }, [isStatusPending, atLeastOneConnected]); + const [wasConfiguredOnMount, setWasConfiguredOnMount] = useState<boolean | null>(null); + useEffect(() => { + if (!isStatusPending && wasConfiguredOnMount === null) { + setWasConfiguredOnMount(atLeastOneConnected); + } + }, [isStatusPending, atLeastOneConnected, wasConfiguredOnMount]);- const shouldAutoAdvance = - !completed && !manualWalkthrough && wasConfiguredOnMount.current === true; + const shouldAutoAdvance = + !completed && !manualWalkthrough && wasConfiguredOnMount === true;
setWasConfiguredOnMountfires exactly once (guarded by=== null), schedules a re-render, andshouldAutoAdvanceevaluates correctly in the subsequent render.Also applies to: 55-56
🤖 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/setup/providers/page.tsx` around lines 39 - 44, The issue is that wasConfiguredOnMount is a useRef so setting wasConfiguredOnMount.current inside the effect doesn't trigger a re-render and prevents shouldAutoAdvance from ever becoming true; replace the useRef usage with useState (e.g., const [wasConfiguredOnMount, setWasConfiguredOnMount] = useState<boolean|null>(null)), update the effect to call setWasConfiguredOnMount(atLeastOneConnected) guarded by wasConfiguredOnMount === null so it only fires once, and update any subsequent reads of wasConfiguredOnMount.current to use the state variable so shouldAutoAdvance recomputes and the auto-advance effect can navigate as intended (also apply the same change for the second occurrence referenced in the review).
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx (1)
27-35: ⚡ Quick winAuth query error states are silently swallowed.
Neither
isErrornorerroris destructured from the twouseQuerycalls. When either query fails (network error, service down, etc.),anthropicAuthStatus/openAIAuthStatusareundefined, both providers appear as "not connected", and the user gets no indication something went wrong.Consider surfacing a brief inline error with
select-text cursor-textclasses per the project's coding guidelines for renderer error text.♻️ Suggested approach
- const { data: anthropicAuthStatus, isPending: isAnthropicPending } = - chatServiceTrpc.auth.getAnthropicStatus.useQuery(); - const { data: openAIAuthStatus, isPending: isOpenAIPending } = - chatServiceTrpc.auth.getOpenAIStatus.useQuery(); + const { + data: anthropicAuthStatus, + isPending: isAnthropicPending, + isError: isAnthropicError, + } = chatServiceTrpc.auth.getAnthropicStatus.useQuery(); + const { + data: openAIAuthStatus, + isPending: isOpenAIPending, + isError: isOpenAIError, + } = chatServiceTrpc.auth.getOpenAIStatus.useQuery();Then in the render, when
isAnthropicError || isOpenAIError, show a small error notice:{(isAnthropicError || isOpenAIError) && ( <p className="select-text cursor-text text-[11px] text-red-400"> Failed to load provider status. Please restart the app and try again. </p> )}As per coding guidelines, error text must carry
select-text cursor-textbecause the renderer setsuser-select: noneonbody.🤖 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/setup/providers/page.tsx` around lines 27 - 35, The auth queries (chatServiceTrpc.auth.getAnthropicStatus.useQuery and chatServiceTrpc.auth.getOpenAIStatus.useQuery) currently only destructure data/isPending, so network/query errors are swallowed; update both calls to also destructure isError and/or error (e.g., isAnthropicError, anthropicError, isOpenAIError, openAIError) and use those flags in the render to show a small inline error when either is true, e.g., render a <p> with classes "select-text cursor-text text-[11px] text-red-400" that says the status failed to load and suggests restarting the app; ensure existing connected checks (claudeConnected, codexConnected) still use anthropicAuthStatus/openAIAuthStatus but do not hide the error message when the queries errored.
🤖 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 `@packages/chat/src/server/desktop/chat-service/openai-oauth-loopback.ts`:
- Around line 39-93: start() can leave a listening socket open if stop() is
called before listen() completes; add a pendingServer and stopRequested flag to
track the server created in start() (before calling listen) and to allow stop()
to close it. Specifically: in start(), assign the created server to a local
pendingServer and register the error listener; when calling listen(), in the
listen callback check stopRequested — if true, close pendingServer and don't set
this.server; otherwise set this.server and clear pendingServer; in stop(), set
stopRequested = true, close and clear pendingServer if present, and if
this.server exists close and null it; ensure you still remove the onListenError
listener on successful listen or when closing pendingServer so no leaks occur.
- Around line 15-30: The parser currently discards the redirect hostname; update
LoopbackTarget and LoopbackOptions to include a host:string and modify
parseLoopbackTargetFromAuthUrl to return the validated redirectUri.hostname
(preserving "localhost", "127.0.0.1" or "[::1]") alongside port and path; then
thread that host through to wherever start() constructs the listen options and
call server.listen(options.port, options.host) instead of hardcoding "127.0.0.1"
so the server binds to the same loopback address the OAuth redirect expects.
---
Outside diff comments:
In `@apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx`:
- Around line 39-44: The issue is that wasConfiguredOnMount is a useRef so
setting wasConfiguredOnMount.current inside the effect doesn't trigger a
re-render and prevents shouldAutoAdvance from ever becoming true; replace the
useRef usage with useState (e.g., const [wasConfiguredOnMount,
setWasConfiguredOnMount] = useState<boolean|null>(null)), update the effect to
call setWasConfiguredOnMount(atLeastOneConnected) guarded by
wasConfiguredOnMount === null so it only fires once, and update any subsequent
reads of wasConfiguredOnMount.current to use the state variable so
shouldAutoAdvance recomputes and the auto-advance effect can navigate as
intended (also apply the same change for the second occurrence referenced in the
review).
---
Nitpick comments:
In `@apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsx`:
- Around line 27-35: The auth queries
(chatServiceTrpc.auth.getAnthropicStatus.useQuery and
chatServiceTrpc.auth.getOpenAIStatus.useQuery) currently only destructure
data/isPending, so network/query errors are swallowed; update both calls to also
destructure isError and/or error (e.g., isAnthropicError, anthropicError,
isOpenAIError, openAIError) and use those flags in the render to show a small
inline error when either is true, e.g., render a <p> with classes "select-text
cursor-text text-[11px] text-red-400" that says the status failed to load and
suggests restarting the app; ensure existing connected checks (claudeConnected,
codexConnected) still use anthropicAuthStatus/openAIAuthStatus but do not hide
the error message when the queries errored.
🪄 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
Run ID: 51535122-11bc-45fe-a296-b610f9a9235f
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/setup/providers/page.tsxpackages/chat/src/server/desktop/chat-service/chat-service.tspackages/chat/src/server/desktop/chat-service/openai-oauth-loopback.ts
Comment on lines
+39
to
+93
| async start(options: LoopbackOptions): Promise<void> { | ||
| return new Promise<void>((resolve, reject) => { | ||
| const server = createServer((req, res) => { | ||
| try { | ||
| const requestUrl = new URL( | ||
| req.url ?? "/", | ||
| `http://127.0.0.1:${options.port}`, | ||
| ); | ||
| if (requestUrl.pathname !== options.path) { | ||
| res.writeHead(404, { "content-type": "text/plain" }); | ||
| res.end("Not found"); | ||
| return; | ||
| } | ||
|
|
||
| const error = requestUrl.searchParams.get("error"); | ||
| const code = requestUrl.searchParams.get("code"); | ||
| if (error || !code) { | ||
| const message = error ?? "Missing authorization code"; | ||
| res.writeHead(400, { "content-type": "text/html; charset=utf-8" }); | ||
| res.end(renderErrorPage(message)); | ||
| options.onError?.(new Error(message)); | ||
| return; | ||
| } | ||
|
|
||
| res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); | ||
| res.end(SUCCESS_PAGE); | ||
| options.onCallback(requestUrl.toString()); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| try { | ||
| res.writeHead(500, { "content-type": "text/plain" }); | ||
| res.end(message); | ||
| } catch { | ||
| // Response may have already been sent — ignore. | ||
| } | ||
| options.onError?.(err instanceof Error ? err : new Error(message)); | ||
| } | ||
| }); | ||
|
|
||
| const onListenError = (err: Error) => reject(err); | ||
| server.once("error", onListenError); | ||
| server.listen(options.port, "127.0.0.1", () => { | ||
| server.off("error", onListenError); | ||
| this.server = server; | ||
| resolve(); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| stop(): void { | ||
| if (this.server) { | ||
| this.server.close(); | ||
| this.server = null; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/chat/src/server/desktop/chat-service/openai-oauth-loopback.tsRepository: superset-sh/superset
Length of output: 5022
🏁 Script executed:
# Also check if there are any tests or usage patterns
rg -A 5 -B 5 "OpenAIOAuthLoopback" packages/chat --type ts --type tsxRepository: superset-sh/superset
Length of output: 92
🏁 Script executed:
# Check the full class definition to understand initialization
ast-grep --pattern $'export class OpenAIOAuthLoopback {
$$$
}'Repository: superset-sh/superset
Length of output: 6150
Cancel pending start() when stop() is called.
this.server is only populated after listen() succeeds. If stop() is called before the listen callback fires, the pending server is never stored or closed, leaving a stale listening socket that can receive requests and invoke callbacks after the promise resolves.
Add tracking for the pending server so that calling stop() before listen completes will close or discard the listener before it becomes active. The suggested approach using pendingServer and a stopRequested flag correctly handles this:
- Store the server in
pendingServerbefore callinglisten() - Check the
stopRequestedflag in the listen callback - If stop was requested, close the server without storing it in
this.server - This ensures cleanup regardless of timing
🤖 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 `@packages/chat/src/server/desktop/chat-service/openai-oauth-loopback.ts`
around lines 39 - 93, start() can leave a listening socket open if stop() is
called before listen() completes; add a pendingServer and stopRequested flag to
track the server created in start() (before calling listen) and to allow stop()
to close it. Specifically: in start(), assign the created server to a local
pendingServer and register the error listener; when calling listen(), in the
listen callback check stopRequested — if true, close pendingServer and don't set
this.server; otherwise set this.server and clear pendingServer; in stop(), set
stopRequested = true, close and clear pendingServer if present, and if
this.server exists close and null it; ensure you still remove the onListenError
listener on successful listen or when closing pendingServer so no leaks occur.
Reconfigure flows previously got stuck on the OAuth subroute because the navigate-back effect required wasAuthedOnMount to be false — true for any reconnect. - Drive both Claude and Codex back to /setup/providers from the OAuth hook's onAuthStateChange callback (fires whether the credential was fresh or refreshed). Drop the wasAuthedOnMount gating. - Wire the OpenAI loopback through the renderer: the server stashes the callback URL, the renderer polls a new consumeOpenAIOAuthCallback query while the dialog is open and runs the existing completeOpenAIOAuth mutation, so the dialog dismissal + navigate-back path is the same for manual paste and auto-capture. - Add onAuthStateChange parameter to useOpenAIOAuth (already existed on useAnthropicOAuth).
… IPv4 parseLoopbackTargetFromAuthUrl now preserves the host from the redirect URI (stripping IPv6 brackets) and threads it through to server.listen, so an OpenAI client configured with [::1] or a system that resolves localhost to IPv6 won't fail to bind.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/chat/src/server/desktop/chat-service/chat-service.ts`:
- Around line 411-414: consumeOpenAIOAuthCallback currently clears
pendingOpenAIOAuthCallbackUrl immediately, which loses the callback if
completeOpenAIOAuth fails; change the behavior so the
pendingOpenAIOAuthCallbackUrl is not cleared by consumeOpenAIOAuthCallback
itself and instead is cleared only on successful completion (i.e., clear inside
completeOpenAIOAuth on success) or add an explicit ack method (e.g.,
ackOpenAIOAuthCallback) that the renderer calls after success; update references
to pendingOpenAIOAuthCallbackUrl, consumeOpenAIOAuthCallback(),
completeOpenAIOAuth(), and any polling code to use the new non-destructive
consume + explicit clear or clear-on-success flow.
- Around line 389-405: The current bare catch around loopback.start() swallows
all errors; change it to only silence expected bind failures (e.g., EADDRINUSE,
EACCES) and rethrow or log unexpected errors. Specifically, wrap the await
loopback.start(...) in try/catch, inspect the caught error.code (or instance)
inside the catch: if it's an address/bind error (EADDRINUSE, EACCES, etc.) call
loopback.stop() and proceed with the manual-paste fallback, otherwise log the
error via this.logger.error (or rethrow) so unexpected failures in
loopback.start() are surfaced; keep the assignment this.openAIOAuthLoopback =
loopback only on success.
🪄 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
Run ID: 2e4f1b3e-483f-442f-8fcd-abbd121b9cd8
📒 Files selected for processing (5)
apps/desktop/src/renderer/components/Chat/ChatInterface/components/ModelPicker/hooks/useOpenAIOAuth/useOpenAIOAuth.tsapps/desktop/src/renderer/routes/_authenticated/setup/providers/claude-code/page.tsxapps/desktop/src/renderer/routes/_authenticated/setup/providers/codex/page.tsxpackages/chat/src/server/desktop/chat-service/chat-service.tspackages/chat/src/server/desktop/router/router.ts
Comment on lines
+389
to
+405
| try { | ||
| await loopback.start({ | ||
| port: target.port, | ||
| path: target.path, | ||
| onCallback: (callbackUrl) => { | ||
| // Stash the callback URL so the renderer can consume it on its | ||
| // next poll. The renderer drives completion through the same | ||
| // completeOpenAIOAuth mutation as the manual-paste flow, so | ||
| // the dialog dismissal + navigation behavior stays consistent. | ||
| this.pendingOpenAIOAuthCallbackUrl = callbackUrl; | ||
| }, | ||
| }); | ||
| this.openAIOAuthLoopback = loopback; | ||
| } catch { | ||
| // Port unavailable or other bind failure — fall back to manual paste. | ||
| loopback.stop(); | ||
| } |
Contributor
There was a problem hiding this comment.
Only silence expected bind failures here.
This bare catch turns any bug inside OpenAIOAuthLoopback.start() into a silent fallback, so the auto-capture path can break without surfacing anywhere. Restrict the silent path to expected bind errors and rethrow or at least log unexpected failures.
Suggested tightening
- } catch {
- // Port unavailable or other bind failure — fall back to manual paste.
- loopback.stop();
- }
+ } catch (error) {
+ loopback.stop();
+ const code = (error as NodeJS.ErrnoException | undefined)?.code;
+ if (code === "EADDRINUSE" || code === "EACCES") {
+ // Port unavailable — fall back to manual paste.
+ return result;
+ }
+ throw error;
+ }🤖 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 `@packages/chat/src/server/desktop/chat-service/chat-service.ts` around lines
389 - 405, The current bare catch around loopback.start() swallows all errors;
change it to only silence expected bind failures (e.g., EADDRINUSE, EACCES) and
rethrow or log unexpected errors. Specifically, wrap the await
loopback.start(...) in try/catch, inspect the caught error.code (or instance)
inside the catch: if it's an address/bind error (EADDRINUSE, EACCES, etc.) call
loopback.stop() and proceed with the manual-paste fallback, otherwise log the
error via this.logger.error (or rethrow) so unexpected failures in
loopback.start() are surfaced; keep the assignment this.openAIOAuthLoopback =
loopback only on success.
Comment on lines
+411
to
+414
| consumeOpenAIOAuthCallback(): { callbackUrl: string | null } { | ||
| const callbackUrl = this.pendingOpenAIOAuthCallbackUrl; | ||
| this.pendingOpenAIOAuthCallbackUrl = null; | ||
| return { callbackUrl }; |
Contributor
There was a problem hiding this comment.
Don't clear the captured callback before completion succeeds.
consumeOpenAIOAuthCallback() is destructive. If the renderer polls this value and completeOpenAIOAuth() then fails, the loopback redirect is gone and the user has to restart OAuth instead of retrying with the captured callback. Keep it until completion succeeds, or add an explicit ack/clear step after success.
🤖 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 `@packages/chat/src/server/desktop/chat-service/chat-service.ts` around lines
411 - 414, consumeOpenAIOAuthCallback currently clears
pendingOpenAIOAuthCallbackUrl immediately, which loses the callback if
completeOpenAIOAuth fails; change the behavior so the
pendingOpenAIOAuthCallbackUrl is not cleared by consumeOpenAIOAuthCallback
itself and instead is cleared only on successful completion (i.e., clear inside
completeOpenAIOAuth on success) or add an explicit ack method (e.g.,
ackOpenAIOAuthCallback) that the renderer calls after success; update references
to pendingOpenAIOAuthCallbackUrl, consumeOpenAIOAuthCallback(),
completeOpenAIOAuth(), and any polling code to use the new non-destructive
consume + explicit clear or clear-on-success flow.
Replace the standalone PNG and the placeholder bracket SVG with the actual brand glyph extracted from packages/ui's preset-icons/superset.svg. Render it inside a CSS-styled tile so all three provider tiles (Superset / Claude / Codex) share identical 44x44 rounded-[10px] boxes, and update the project page thumbnail to a rounded-square 48px tile with a larger glyph.
Add per-worktree checkboxes to the adopt-worktrees onboarding step so users can choose which worktrees to bring in instead of being forced into all-or-nothing. New importExternalWorktrees tRPC mutation accepts an explicit paths array and shares its filter logic with importAllWorktrees through a new selectExternalWorktreesForImport helper.
…mport Have getExternalWorktrees call selectExternalWorktreesForImport so the list shown in the UI cannot drift from the rules applied during import.
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
* feat(desktop): remove skip-all onboarding affordance Per-step skipping is enough — drop the chrome-level "Skip onboarding" button and the associated `skipAll` store action. Each step's "Skip for now" link is unchanged. * feat(desktop): unify onboarding step layout and stack providers - Show Claude Code and Codex on the same providers page (no more tab toggle); each has its own selectable connection methods and Connect button, stacked vertically. - Ensure every step renders both Continue (primary) and "Skip for now" (link) in a stable bottom column. Adds the missing skip on the gh-cli installed branch and the adopt-worktrees empty branch. * feat(desktop): auto-capture OpenAI OAuth callback via loopback - Add a localhost loopback HTTP server that binds to the redirect_uri port (1455) advertised in the OpenAI authorize URL. When the browser hits /auth/callback, the server passes the URL straight to completeOpenAIOAuth so the user no longer has to copy/paste it. Falls back silently to the existing manual paste flow if the port can't be bound. - Tidy the providers page reconfigure UX: move Reconfigure/Cancel to a small inline link in the section header so the in-flow stack is just option cards + a single primary button instead of three stacked buttons. * fix(desktop): route back to providers page after reconnect completes Reconfigure flows previously got stuck on the OAuth subroute because the navigate-back effect required wasAuthedOnMount to be false — true for any reconnect. - Drive both Claude and Codex back to /setup/providers from the OAuth hook's onAuthStateChange callback (fires whether the credential was fresh or refreshed). Drop the wasAuthedOnMount gating. - Wire the OpenAI loopback through the renderer: the server stashes the callback URL, the renderer polls a new consumeOpenAIOAuthCallback query while the dialog is open and runs the existing completeOpenAIOAuth mutation, so the dialog dismissal + navigate-back path is the same for manual paste and auto-capture. - Add onAuthStateChange parameter to useOpenAIOAuth (already existed on useAnthropicOAuth). * fix(desktop): bind OAuth loopback to redirect_uri host, not hardcoded IPv4 parseLoopbackTargetFromAuthUrl now preserves the host from the redirect URI (stripping IPv6 brackets) and threads it through to server.listen, so an OpenAI client configured with [::1] or a system that resolves localhost to IPv6 won't fail to bind. * feat(desktop): use real app icon on connect-provider pages Swap the inline SupersetIcon SVG (bracket marks rendered as text on a flat circle) for the actual macOS app icon PNG, so the Connect Claude Code and Connect Codex headers show the rounded-square brand mark users recognize from the dock. * style(desktop): round superset icon on connect pages to match provider logos * style(desktop): scale superset icon so it fills the circular crop * style(desktop): use inline SupersetIcon SVG on connect pages The macOS app-icon PNG has rounded-square padding and a bottom shadow baked in, which looked off when shoehorned into a circular badge next to the Claude/Codex logos. Switch back to the SVG bracket mark on a flat circular dark background — crisp at any size and visually consistent with the provider icons. * feat(desktop): use docs logo.png for connect provider pages Add a copy of apps/docs/public/logo.png at apps/desktop/src/renderer/ assets/superset-logo.png so the desktop app has its own copy, and use it as the Superset mark in Connect Claude Code and Connect Codex. Renders at the logo's native rounded-square shape. * style(desktop): square off provider logos to match superset icon shape * style(desktop): match superset icon size and squared border container * style(desktop): bump superset logo scale so brackets match codex/claude size * style(desktop): leave superset logo at natural size, bump codex/claude inner icons * style(desktop): use real superset brand glyph for setup tiles Replace the standalone PNG and the placeholder bracket SVG with the actual brand glyph extracted from packages/ui's preset-icons/superset.svg. Render it inside a CSS-styled tile so all three provider tiles (Superset / Claude / Codex) share identical 44x44 rounded-[10px] boxes, and update the project page thumbnail to a rounded-square 48px tile with a larger glyph. * feat(desktop): selectable worktree import in onboarding Add per-worktree checkboxes to the adopt-worktrees onboarding step so users can choose which worktrees to bring in instead of being forced into all-or-nothing. New importExternalWorktrees tRPC mutation accepts an explicit paths array and shares its filter logic with importAllWorktrees through a new selectExternalWorktreesForImport helper. * refactor(desktop): share external-worktree filter between query and import Have getExternalWorktrees call selectExternalWorktreesForImport so the list shown in the UI cannot drift from the rules applied during import.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…o-v2 superset-sh#4113 remove skip-all onboarding affordance: integrate fork's githubSyncService import alongside upstream's selectExternalWorktreesForImport in workspaces/procedures/git-status.ts. superset-sh#4115 default new users to v2 + v2 banner in v1: - Adopt upstream's V2_CLOUD flag removal: collapse useIsV2CloudEnabled to a plain boolean (drop isRemoteV2Enabled) and update every callsite back to scalar destructure (TopBar, settings/{behavior,experimental, git,links,terminal}/page, SettingsSidebar/{GeneralSettings,SettingsSidebar}). - ExperimentalSettings.tsx: drop isRemoteV2Enabled-gated 'Early access not enabled' note and Switch disabled prop; simplify track call. - v2-local-override.ts: keep fork's IS_DEV strong default (dev installs land on v2) alongside upstream's hasPriorSupersetUsage probe (initialOptInV2 = IS_DEV ? true : !hasPriorSupersetUsage()). - TopBar.tsx: integrate upstream's useWorkspaceSidebarStore reads with fork's existing chrome layout. - useMigrateV1DataToV2/useMigrateV1DataToV2.ts: re-delete (DU; upstream attempted to revive via superset-sh#4115 vocabulary cleanup; cycle 38 rationale still applies — fork trpc lacks the required procedures). superset-sh#4125 onboarding auto-skip drop, SetupButton design alignment: clean cherry-pick. superset-sh#4175 keep Skip-for-now visible during loading: clean cherry-pick. Replaces v2Projects live query with imperative v2Project.list trpc call; backfill list procedure (jwtProcedure variant) into fork's v2-project router (mirrors get pattern using ctx.organizationIds membership check). superset-sh#4214 stable Adopt button + Adopt-all (skipped): touches v1 ImportWorkspacesPage/V1ImportModal which the fork has removed. The upstream improvements (idempotent adopt-all guard, smarter skip-when- already-running) have no shipping surface in this fork; pick up if and when v1 import modal is reintroduced. superset-sh#4093 re-add Settings to org dropdown for v1: clean cherry-pick.
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
1 participant
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.
Summary
OnboardingProgressand drop theskipAllaction (plus itsskipped_allanalytics event) from the onboarding storemarkSkipped()are unchanged — users can still skip individual stepsTest plan
/welcomeuntil completed or skippedbun run lintandbun run typecheckpassSummary by cubic
Removed the global “Skip onboarding,” stacked Claude Code and Codex with per‑provider Connect and inline Reconfigure/Cancel, and standardized bottom Continue/“Skip for now” across steps (incl.
gh-cliandadopt-worktrees). Added OpenAI OAuth localhost loopback that auto‑completes via renderer polling and refreshed connect headers with the Superset brand glyph in 44×44 tiles matching squared provider logos.New Features
consumeOpenAIOAuthCallbackto complete auth and routes back to/setup/providers. Loopback stops on cancel/complete and falls back if the port can’t bind.adopt-worktreesvia checkboxes (with Select all); newworkspaces.importExternalWorktreesacceptspaths, andselectExternalWorktreesForImportis used by both the query and import to keep the UI in sync (with tests).Bug Fixes
redirect_urihost (supportslocalhost,127.0.0.1, and::1) to prevent bind failures on IPv6 systems.onAuthStateChangeback to/setup/providers.Written for commit a3bb042. Summary will update on new commits.
Summary by CodeRabbit
Chores
New Features
Refactor