fix(gui): Cursor OAuth accounts stay visible + toast login status - #1257
Conversation
Stop the Providers login banner from shifting layout, seed/refresh the account list after OAuth, and open Accounts so multi-account Cursor logins stay visible.
|
✅ Deterministic PR hygiene checks passed. |
|
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 PR adds multi-account provider controls, forced Cursor account selection, OAuth account-state tracking, Accounts-tab navigation, dismissible toast notifications, and localized same-account login messages. ChangesMulti-account OAuth flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderCatalog
participant Providers
participant useProvidersOAuth
participant CursorOAuth
participant ProviderDetails
ProviderCatalog->>Providers: Start add-account login
Providers->>useProvidersOAuth: Pass addAccount flag
useProvidersOAuth->>CursorOAuth: Request forced account login
CursorOAuth-->>useProvidersOAuth: Return OAuth status and account data
useProvidersOAuth-->>Providers: Settle login and refresh accounts
Providers->>ProviderDetails: Focus Accounts tab
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@gui/src/components/AddProviderModal.tsx`:
- Line 252: Update the account-management flow used by AddProviderModal: ensure
the callback passed through onManage closes the add-provider modal via
onCloseAdd before or while invoking revealProviderAccounts(provider). Keep the
existing account-reveal behavior intact and ensure adding is cleared before
navigation.
In `@gui/src/components/provider-catalog/ProviderCatalog.tsx`:
- Around line 184-197: Update the provider row actions around onLogin/onLogout
so an OAuth login in progress remains mutually exclusive with logout: preserve
the waiting label, render the row’s onCancelLogin action while busy, and hide or
disable Logout until the login settles. In logoutOAuth, invalidate
oauthLoginGenerationRef before proceeding with logout so any active polling or
completion cannot update the account afterward, while keeping GUI state
synchronized with the management API result.
In `@gui/src/components/provider-workspace/ProviderDetails.tsx`:
- Around line 119-124: Update the accounts-focus useEffect to call
switchTab("accounts") instead of setTab("accounts"), preserving the existing
token and authSurface guards so unsaved Settings changes trigger
UnsavedLeaveDialog before navigation.
In `@gui/src/pages/use-providers-oauth.ts`:
- Around line 139-146: Update the state construction inside the status-response
handler to derive activeAccountId from the active OAuthAccount when
s.activeAccountId is absent, using null only when neither source provides an
active account. Preserve the existing s.activeAccountId value when present and
continue storing s.accounts unchanged in the AccountSet.
In `@gui/src/ui.tsx`:
- Around line 30-39: Remove the default `"Close"` value from ToastNotice’s
dismissLabel prop and make dismissLabel required. Update every ToastNotice
caller to pass the localized t("common.close") value explicitly, ensuring no
hardcoded dismissal text remains.
In `@src/oauth/cursor.ts`:
- Around line 67-70: Update cursorJwtIdentity to accept numeric JWT subjects
only when Number.isSafeInteger(value) is true, rejecting unsafe integers while
preserving existing non-empty string handling and safe-number string conversion.
- Around line 79-86: Remove the unsupported prompt=login parameter from
generateCursorAuthParams and stop treating forceLogin as a server-controlled
account switch. Preserve only documented Cursor authentication parameters, and
update the forceLogin flow/instructions to require logout and manual re-login
before adding another account, or remove forceLogin if no supported behavior
exists.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30d1f947-4efe-40bf-a13f-54ea59245729
📒 Files selected for processing (18)
gui/src/components/AddProviderModal.tsxgui/src/components/provider-catalog/ProviderCatalog.tsxgui/src/components/provider-workspace/ProviderDetails.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Providers.tsxgui/src/pages/providers-page-modals.tsxgui/src/pages/use-providers-oauth.tsgui/src/styles.cssgui/src/ui.tsxsrc/oauth/cursor.tssrc/oauth/index.tstests/cursor-oauth.test.tstests/oauth-store-multi.test.ts
| export function ToastNotice({ | ||
| tone, | ||
| children, | ||
| onDismiss, | ||
| dismissLabel = "Close", | ||
| }: { | ||
| tone: "ok" | "err"; | ||
| children: ReactNode; | ||
| onDismiss?: () => void; | ||
| dismissLabel?: string; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the hardcoded dismissal label.
Line 34 exposes "Close" when a caller omits dismissLabel. This bypasses localization and can show English in another locale.
Require dismissLabel and keep passing t("common.close") from each caller.
Proposed fix
- dismissLabel = "Close",
+ dismissLabel,
@@
- dismissLabel?: string;
+ dismissLabel: string;As per path instructions, user-visible strings must go through the i18n locale files rather than hardcoded text.
📝 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.
| export function ToastNotice({ | |
| tone, | |
| children, | |
| onDismiss, | |
| dismissLabel = "Close", | |
| }: { | |
| tone: "ok" | "err"; | |
| children: ReactNode; | |
| onDismiss?: () => void; | |
| dismissLabel?: string; | |
| export function ToastNotice({ | |
| tone, | |
| children, | |
| onDismiss, | |
| dismissLabel, | |
| }: { | |
| tone: "ok" | "err"; | |
| children: ReactNode; | |
| onDismiss?: () => void; | |
| dismissLabel: string; |
🤖 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 `@gui/src/ui.tsx` around lines 30 - 39, Remove the default `"Close"` value from
ToastNotice’s dismissLabel prop and make dismissLabel required. Update every
ToastNotice caller to pass the localized t("common.close") value explicitly,
ensuring no hardcoded dismissal text remains.
Source: Path instructions
Resolve Providers/OAuth hook conflicts against upstream, then tighten Cursor force-login (no unsupported prompt=), logout/login exclusivity, Accounts tab focus via switchTab, and ToastNotice dismiss labeling.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@gui/src/pages/use-providers-oauth.ts`:
- Around line 120-122: In the add-account/reauth completion logic around
statusCount and the result handling near prov.loginSameAccount, preserve the
distinction between an omitted accounts list and an empty list instead of
coercing accounts?.length to 0. Only infer a duplicate account when the status
response includes accounts, or defer the comparison until refreshed state from
fetchAccountSets() is available; do not report prov.loginSameAccount solely
because s.done is true and the baseline count is positive.
In `@tests/cursor-oauth.test.ts`:
- Around line 164-170: Extend the existing loginCursor forceLogin regression
test to capture OAuthController.onAuth.instructions and assert it includes the
account-switching guidance, including logout and private-window instructions.
Keep the current URL parameter assertions unchanged and place the focused
coverage alongside the existing cursor OAuth tests.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 68e45b4b-6ffb-4454-9a56-b803452105cd
📒 Files selected for processing (18)
gui/src/components/AddProviderModal.tsxgui/src/components/provider-catalog/ProviderCatalog.tsxgui/src/components/provider-workspace/ProviderDetails.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Providers.tsxgui/src/pages/providers-page-modals.tsxgui/src/pages/use-providers-oauth.tsgui/src/styles.cssgui/src/ui.tsxsrc/oauth/cursor.tssrc/oauth/index.tstests/cursor-oauth.test.tstests/oauth-store-multi.test.ts
| const statusCount = s.accounts?.length ?? 0; | ||
| const completed = addAccount || reauthTargetId | ||
| ? ((s.accounts?.length ?? 0) > baselineCount || s.done === true) | ||
| ? (statusCount > baselineCount || s.done === true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not infer a duplicate account from an omitted account list.
accounts is optional in the status response. Line 120 converts an unknown count to 0. If s.done completes an add-account login, Line 159 then reports prov.loginSameAccount whenever the baseline count is positive, even when the refresh added a new account.
Keep statusCount optional. Only report the duplicate-account result when the status response contains an account list, or compare the refreshed account state after fetchAccountSets() resolves.
Proposed fix
- const statusCount = s.accounts?.length ?? 0;
+ const statusCount = s.accounts?.length;
const completed = addAccount || reauthTargetId
- ? (statusCount > baselineCount || s.done === true)
+ ? ((statusCount !== undefined && statusCount > baselineCount) || s.done === true)
: (s.loggedIn || s.done === true);
@@
- const sameIdentityAdd = addAccount && !reauthTargetId && statusCount <= baselineCount;
+ const sameIdentityAdd = addAccount
+ && !reauthTargetId
+ && statusCount !== undefined
+ && statusCount <= baselineCount;Also applies to: 159-163
🤖 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 `@gui/src/pages/use-providers-oauth.ts` around lines 120 - 122, In the
add-account/reauth completion logic around statusCount and the result handling
near prov.loginSameAccount, preserve the distinction between an omitted accounts
list and an empty list instead of coercing accounts?.length to 0. Only infer a
duplicate account when the status response includes accounts, or defer the
comparison until refreshed state from fetchAccountSets() is available; do not
report prov.loginSameAccount solely because s.done is true and the baseline
count is positive.
Source: Path instructions
| test("generateCursorAuthParams keeps the documented PKCE URL even when forceLogin is set", async () => { | ||
| const p = await generateCursorAuthParams({ forceLogin: true }); | ||
| const url = new URL(p.loginUrl); | ||
| expect(url.searchParams.get("prompt")).toBeNull(); | ||
| expect(url.searchParams.get("mode")).toBe("login"); | ||
| expect(url.searchParams.get("redirectTarget")).toBe("cli"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression for the force-login instructions.
The new branch in src/oauth/cursor.ts at Lines [161-163] changes OAuthController.onAuth.instructions. This test checks only URL parameters. It will not detect a regression in the logout or private-window guidance.
Extend the existing loginCursor test to capture instructions with forceLogin: true and assert the account-switching guidance.
As per path instructions, behavior changes in src/** should have a focused regression test near the existing tests for that subsystem.
🤖 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 `@tests/cursor-oauth.test.ts` around lines 164 - 170, Extend the existing
loginCursor forceLogin regression test to capture
OAuthController.onAuth.instructions and assert it includes the account-switching
guidance, including logout and private-window instructions. Keep the current URL
parameter assertions unchanged and place the focused coverage alongside the
existing cursor OAuth tests.
Source: Path instructions
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38f33f025f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!accountsFocusToken || accountsFocusToken === lastAccountsFocusTokenRef.current) return; | ||
| lastAccountsFocusTokenRef.current = accountsFocusToken; | ||
| if (!authSurface) return; | ||
| setTab("accounts"); |
There was a problem hiding this comment.
When an OAuth login remains pending while the user opens Settings and edits the provider, completion calls setTab("accounts") directly. This bypasses the switchTab dirty-state guard immediately above, unmounts ProviderSettings, and discards the draft without showing UnsavedLeaveDialog; route this transition through the existing guarded tab-switch path instead.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| fetchProviderQuotas(true); | ||
| await fetchAccountSets(knownSet.has(provider) ? knownProviders : [...knownProviders, provider]); | ||
| if (!aliveRef.current || oauthLoginGenerationRef.current.get(provider) !== generation) return; | ||
| const sameIdentityAdd = addAccount && !reauthTargetId && statusCount <= baselineCount; |
There was a problem hiding this comment.
When accounts A and B are already saved, A is active, and Add account signs into B, the store activates/replaces B without increasing the account count. This condition therefore reports “Still the same account” even though the identity and active account changed; compare the resulting active/account identity with the pre-login identity instead of using only the list length.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| onLogout && <button type="button" className="btn btn-ghost" onClick={() => onLogout(row.id)}>{t("modal.accountLogout")}</button> | ||
| <> | ||
| {onManage && ( | ||
| <button type="button" className="btn btn-ghost" onClick={() => onManage(row.id)}> |
There was a problem hiding this comment.
Deleting an OAuth provider removes its config row but deliberately leaves its stored credential, so Add Provider → Accounts still considers that provider logged in. In that state this new Manage button closes the modal and selects a provider absent from config.providers; ProviderWorkspaceShell cannot resolve a detail item and only shows the overview. Only offer Manage for configured providers, or recreate/fetch the provider row before navigating.
AGENTS.md reference: gui/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
Adjust the Accounts tab during render when accountsFocusToken changes, matching React's recommended pattern and clearing the CI warning.
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 `@gui/src/components/provider-workspace/ProviderDetails.tsx`:
- Line 97: Initialize seenAccountsFocusToken to 0 in ProviderDetails instead of
accountsFocusToken, so a non-zero token present at mount remains pending. In the
accounts-focus handling logic, only advance the seen token after authSurface is
available and the Accounts tab switch is performed, preserving login-completion
flows.
🪄 Autofix
❌ Autofix failed (check again to retry)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 37e7c8bd-9dcf-4594-8078-4b4953321a28
📒 Files selected for processing (1)
gui/src/components/provider-workspace/ProviderDetails.tsx
|
An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
Seed seenAccountsFocusToken at 0 and only advance it once authSurface exists so a reveal that mounts ProviderDetails with a non-zero token still opens Accounts.
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 `@gui/src/components/provider-workspace/ProviderDetails.tsx`:
- Around line 97-99: Scope accounts-focus requests to the requesting provider by
passing the target provider alongside accountsFocusToken from Providers and
updating ProviderDetails to process the token only when it matches item.name.
Preserve seenAccountsFocusToken initialization at 0 and ensure unmatched
requests do not become consumed or affect the selected provider; add a
regression test covering provider A requesting focus followed by selecting
provider B.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ab53cb9-e026-4ec5-8c72-cc276c65c889
📒 Files selected for processing (1)
gui/src/components/provider-workspace/ProviderDetails.tsx
A leftover global token must not open Accounts when selecting another provider. Pair the counter with the target name and ignore mismatches; cover with a DOM regression test.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/pages/Providers.tsx (1)
54-71: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestart the success-dismiss timer for repeated messages.
Lines 54-57 can receive the same successful message while that message is already visible. React preserves the identical
statusandstatusOkvalues. Lines 67-71 then keep the timeout from the earlier notification.Track a monotonically increasing notification revision in
notify. Include that revision in the timeout effect dependencies. This makes each successful notification remain visible for 4.5 seconds.🤖 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 `@gui/src/pages/Providers.tsx` around lines 54 - 71, Update notify to increment a monotonically increasing notification revision for every notification, including repeated identical successes. Add that revision to the success-dismiss useEffect dependencies so each successful notify restarts the 4.5-second clearStatus timer, while preserving existing status and error behavior.
🤖 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.
Outside diff comments:
In `@gui/src/pages/Providers.tsx`:
- Around line 54-71: Update notify to increment a monotonically increasing
notification revision for every notification, including repeated identical
successes. Add that revision to the success-dismiss useEffect dependencies so
each successful notify restarts the 4.5-second clearStatus timer, while
preserving existing status and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cfba18ca-f473-4106-b476-0e6c488b1a72
📒 Files selected for processing (3)
gui/src/components/provider-workspace/ProviderDetails.tsxgui/src/pages/Providers.tsxgui/tests/accounts-focus-provider-scope.test.tsx
Bump a status revision on each notify so the auto-dismiss effect re-arms when the same success message is shown again.
1 participant
Summary
Noticewith a fixed toast so success/error feedback no longer shifts the workspace layout; success auto-dismisses after ~4.5s.Test plan
bun test tests/cursor-oauth.test.ts tests/oauth-store-multi.test.tsSummary by CodeRabbit
New Features
Bug Fixes
UI Improvements
Localization