fix(auth): close web and desktop logout contracts - #1487
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughDesktop and web authentication now centralize logout cleanup, align OIDC callback handling with the application base path, configure isolated E2E endpoints, portal the profile menu, and expand unit and end-to-end coverage. ChangesAuthentication and OIDC session flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AgentHubWorkbench
participant useAuth
participant queryClient
participant SessionStorage
User->>AgentHubWorkbench: Select logout
AgentHubWorkbench->>useAuth: logout()
AgentHubWorkbench->>queryClient: Clear private queries
AgentHubWorkbench->>SessionStorage: Remove session and workbench state
AgentHubWorkbench-->>User: Show login UI
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/desktop/src/App.tsx (1)
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRun local logout cleanup even if
logout()rejects.Both
handleLogoutimplementations await the shared auth singleton'slogout()before running local cleanup. The upstreamlogout()contract only swallows errors from its network POST call;clearStoredHubAccessToken()/clearStoredHubRefreshToken()remain unguarded. If either rejects, all local cleanup is skipped and the user is stranded on the authenticated view with no path back to login without a full reload.
app/desktop/src/App.tsx#L91-97: wrapqueryClient.clear(), theWORKBENCH_DATA_MODE_STORAGE_KEYremoval, andsetEntryMode('entry')in afinallyblock aroundawait logout().app/web/src/App.tsx#L184-194: wrapsessionQueryClient.clear(), the local selection/error state resets, andsetShowAuthModal(true)in afinallyblock aroundawait logout().🔧 Proposed fix for app/desktop/src/App.tsx
const handleLogout = useCallback(async (): Promise<void> => { - await logout(); - queryClient.clear(); - window.localStorage.removeItem(WORKBENCH_DATA_MODE_STORAGE_KEY); - setEntryMode('entry'); + try { + await logout(); + } finally { + queryClient.clear(); + window.localStorage.removeItem(WORKBENCH_DATA_MODE_STORAGE_KEY); + setEntryMode('entry'); + } }, [logout, queryClient]);🔧 Proposed fix for app/web/src/App.tsx
const handleLogout = useCallback(async (): Promise<void> => { - await logout(); - sessionQueryClient.clear(); - setSelectedConversationId(undefined); - setSelectedProjectId(undefined); - setAgentActionError(undefined); - setSavingAgentId(undefined); - setDeletingAgentId(undefined); - setShowAuthModal(true); + try { + await logout(); + } finally { + sessionQueryClient.clear(); + setSelectedConversationId(undefined); + setSelectedProjectId(undefined); + setAgentActionError(undefined); + setSavingAgentId(undefined); + setDeletingAgentId(undefined); + setShowAuthModal(true); + } }, [logout, sessionQueryClient, setShowAuthModal]);🤖 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 `@app/desktop/src/App.tsx` at line 1, Update both handleLogout implementations in the desktop and web App components so await logout() is wrapped in a try/finally, with all existing local cleanup moved into finally: query-client clearing, storage removal or local state resets, and restoring the entry/auth modal state. Preserve the shared logout call while ensuring cleanup runs even when it rejects.
🤖 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 `@app/desktop/src/__e2e__/oidc-login.spec.ts`:
- Around line 327-330: In the OIDC login test, update the profile-menu button
locator and logout button locator to accept only the expected translated text
produced by the mocked fixture, removing raw i18n keys such as user.fallbackName
and user.logout from both regular expressions.
In `@app/desktop/src/App.tsx`:
- Around line 91-97: Update handleLogout in App.tsx so cleanup always runs even
when logout() rejects: execute queryClient.clear(), remove
WORKBENCH_DATA_MODE_STORAGE_KEY, and setEntryMode('entry') in a finally path,
while preserving the logout attempt and ensuring the rejection does not prevent
the login gate transition.
In `@app/shared/src/workbench/floating/ProfilePopover.tsx`:
- Around line 167-184: Add Playwright coverage for opening ProfilePopover from a
scrollable, overflow-hidden conversation sidebar, asserting the portal-rendered
menu is visible without clipping and receives pointer events. Run the behavioral
and Visual QA checks at the 1440x810 gate viewport in both light and dark modes,
and include evidence for each mode.
In `@app/web/src/__e2e__/oidc-login.spec.ts`:
- Line 534: Update the logout-button assertions in the OIDC login E2E specs,
including the test around the visible getByRole call and its desktop
counterpart, to match only the supported translated labels and exclude the raw
user.logout i18n key. Preserve the existing localized-language coverage without
allowing unresolved translation keys to satisfy the assertion.
In `@app/web/src/App.tsx`:
- Around line 184-194: Update handleLogout in App.tsx so
sessionQueryClient.clear(), local state resets, and setShowAuthModal(true)
execute even when logout() rejects by placing cleanup in a finally path;
preserve the existing logout attempt and callback dependencies.
---
Outside diff comments:
In `@app/desktop/src/App.tsx`:
- Line 1: Update both handleLogout implementations in the desktop and web App
components so await logout() is wrapped in a try/finally, with all existing
local cleanup moved into finally: query-client clearing, storage removal or
local state resets, and restoring the entry/auth modal state. Preserve the
shared logout call while ensuring cleanup runs even when it rejects.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a7cd229f-23d8-466f-a976-0485815762a4
📒 Files selected for processing (11)
app/desktop/playwright.config.tsapp/desktop/src/App.tsxapp/desktop/src/__e2e__/oidc-login.spec.tsapp/desktop/src/__tests__/App.v4.test.tsxapp/shared/src/workbench/floating/ProfilePopover.test.tsxapp/shared/src/workbench/floating/ProfilePopover.tsxapp/web/playwright.config.tsapp/web/src/App.test.tsxapp/web/src/App.tsxapp/web/src/__e2e__/oidc-login.spec.tsapp/web/src/api/hubAuth.ts
| await page.getByRole('button', { name: /^(Test User|testuser|User|user\.fallbackName)$/ }).click(); | ||
| await expect(page.getByRole('dialog').first()).toBeVisible(); | ||
| await page.screenshot({ path: testInfo.outputPath('profile-menu-open.png'), fullPage: true }); | ||
| await page.getByRole('button', { name: /^(退出登录|Log out|user\.logout)$/ }).click(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Permissive button-name regex accepts raw i18n keys, weakening test protection.
The regex /^(Test User|testuser|User|user\.fallbackName)$/ and /^(退出登录|Log out|user\.logout)$/ both include the raw i18n key text (user.fallbackName, user.logout) as an accepted match alongside the real translated strings. If a translation fails to resolve and the UI falls back to displaying the raw key, these assertions still pass. The test then stops verifying which text actually renders, so it can no longer catch a missing-translation regression. Narrow each assertion to the specific text the mocked fixture (display_name: 'Test User') is expected to produce.
As per coding guidelines, **/*.{test,spec}.{ts,tsx,js,jsx}: "禁止无保护力测试：不要复制实现 switch、测试常量字符串、硬断错误文案，或 mock 被测函数本身；mock 应模拟外部系统。"
🤖 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 `@app/desktop/src/__e2e__/oidc-login.spec.ts` around lines 327 - 330, In the
OIDC login test, update the profile-menu button locator and logout button
locator to accept only the expected translated text produced by the mocked
fixture, removing raw i18n keys such as user.fallbackName and user.logout from
both regular expressions.
Source: Coding guidelines
| const handleLogout = useCallback(async (): Promise<void> => { | ||
| await logout(); | ||
| queryClient.clear(); | ||
| window.localStorage.removeItem(WORKBENCH_DATA_MODE_STORAGE_KEY); | ||
| setEntryMode('entry'); | ||
| }, [logout, queryClient]); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard cleanup against logout() rejecting.
handleLogout awaits logout() before running cleanup. If logout() rejects, queryClient.clear(), the storage-key removal, and setEntryMode('entry') never run. The user stays on the workbench view with a torn-down session and no way back to the login gate without a full reload.
(See consolidated comment for the shared fix with app/web/src/App.tsx.)
🤖 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 `@app/desktop/src/App.tsx` around lines 91 - 97, Update handleLogout in App.tsx
so cleanup always runs even when logout() rejects: execute queryClient.clear(),
remove WORKBENCH_DATA_MODE_STORAGE_KEY, and setEntryMode('entry') in a finally
path, while preserving the logout attempt and ensuring the rejection does not
prevent the login gate transition.
| await page.getByRole('button', { name: 'Web User' }).click(); | ||
| await expect(page.getByRole('dialog').first()).toBeVisible(); | ||
| await page.screenshot({ path: testInfo.outputPath('profile-menu-open.png'), fullPage: true }); | ||
| await page.getByRole('button', { name: /^(退出登录|Log out|user\.logout)$/ }).click(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Permissive logout-button regex accepts the raw i18n key.
/^(退出登录|Log out|user\.logout)$/ accepts the literal i18n key user.logout as a valid match, alongside the two translated strings. If translation resolution breaks and the UI falls back to the raw key, this assertion still passes, so it stops verifying which text renders. The same pattern occurs in app/desktop/src/__e2e__/oidc-login.spec.ts at line 330.
As per coding guidelines, **/*.{test,spec}.{ts,tsx,js,jsx}: "禁止无保护力测试：不要复制实现 switch、测试常量字符串、硬断错误文案，或 mock 被测函数本身；mock 应模拟外部系统。"
🤖 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 `@app/web/src/__e2e__/oidc-login.spec.ts` at line 534, Update the logout-button
assertions in the OIDC login E2E specs, including the test around the visible
getByRole call and its desktop counterpart, to match only the supported
translated labels and exclude the raw user.logout i18n key. Preserve the
existing localized-language coverage without allowing unresolved translation
keys to satisfy the assertion.
Source: Coding guidelines
| const handleLogout = useCallback(async (): Promise<void> => { | ||
| await logout(); | ||
| sessionQueryClient.clear(); | ||
| setSelectedConversationId(undefined); | ||
| setSelectedProjectId(undefined); | ||
| setAgentActionError(undefined); | ||
| setSavingAgentId(undefined); | ||
| setDeletingAgentId(undefined); | ||
| setShowAuthModal(true); | ||
| }, [logout, sessionQueryClient, setShowAuthModal]); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard cleanup against logout() rejecting.
handleLogout awaits logout() before clearing sessionQueryClient and resetting local state. If logout() rejects, none of the cleanup runs, including setShowAuthModal(true). The user stays on the previous authenticated workbench view with no path back to the login modal.
(See consolidated comment for the shared fix with app/desktop/src/App.tsx.)
🤖 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 `@app/web/src/App.tsx` around lines 184 - 194, Update handleLogout in App.tsx
so sessionQueryClient.clear(), local state resets, and setShowAuthModal(true)
execute even when logout() rejects by placing cleanup in a finally path;
preserve the existing logout attempt and callback dependencies.
8740663 to
d5c79fb
Compare
Co-authored-by: Codex <codex@vectorcontrol.tech>
Summary
document.bodyso the logout action is not clipped or intercepted by the conversation sidebarEvidence
git diff --checkpassedKnown baseline debt
tsc --noEmitremains red on pre-existing Storybook/test typing errors outside this slice; the new ProfilePopover test passes independently.Summary by CodeRabbit
Bug Fixes
Tests