fix(oauth): make loopback redirect actually work, plus settings cleanup#2550
Merged
senamakel merged 11 commits intoMay 23, 2026
Merged
Conversation
In dev (pnpm dev:app), the OAuth button was setting both `responseType=json` and `redirectUri=<loopback>` on the backend login URL. `responseType=json` makes the backend return JSON in the browser tab instead of redirecting, so the loopback listener never receives the callback and the app stays stuck on the loading state even though OAuth itself succeeds. Only set `responseType=json` when there is no loopback handle (web build or bind failure), preserving the pre-loopback dev workaround as a fallback.
`start_loopback_oauth_listener` / `stop_loopback_oauth_listener` were registered in the invoke handler (tinyhumansai#2511) but never added to the `allow-core-process` capability allow-list, so every Tauri invoke was rejected with "Command not found" and the loopback path silently fell back to the deep-link redirect on every login click. Adds both commands to the allow-list so the loopback flow can actually fire on desktop.
Five issues that together prevented the loopback OAuth path from ever completing end-to-end in dev: - StartResult serialized as `redirect_uri`/`state` (snake_case) but TS read `result.redirectUri`. The undefined value tripped a silent TypeError in appendState, so every "successful" listener bind was immediately abandoned in JS. Tag with `#[serde(rename_all = "camelCase")]`. - Two clicks in quick succession failed the second bind with EADDRINUSE: `cancel_active_listener` only signalled the previous task; the socket was still owned. Switch to TcpSocket with SO_REUSEADDR + await the previous task's JoinHandle before re-binding. - Fall back to an OS-assigned ephemeral port if the requested port is taken by a stale/unrelated process. The backend `redirectUri` whitelist restricts host but not port, so this is safe. - JS-side `listen()` handler from a previous click stayed registered after Rust cancelled its listener, so the next callback fanned out to every stale handler. Track and unsubscribe before installing a new one. - (Sibling commit) Loopback commands were missing from the `allow-core-process` capability allow-list; every invoke returned "Command not found".
The sun/moon toggle used to sit above the main card in its own row. Move it into the card's header row alongside the version label so the card owns its own controls. The version stays visually centered via a width-matched spacer on the left.
…ntry Notification routing (previously buried in Developer Options) is now a second tab inside the main Settings → Notifications page, alongside the existing per-channel/DnD preferences. The two panels remain independent components but accept an `embedded` flag so the new `NotificationsTabbedPanel` wrapper owns the SettingsHeader and tab strip. The legacy `/settings/notification-routing` path redirects to `/settings/notifications#routing` so existing deep links keep working, and the entry has been removed from Developer Options.
…ions - Extract Log Out / Clear App Data + the confirmation modal out of Settings home into a new LogoutAndClearActions component and render it as the footer of the Settings → Account page. SettingsSectionPage grows an optional `footer` slot. The home menu is now purely navigational; destructive actions live with the rest of the account-scoped controls. - Delete the Connections panel entirely (MetaMask / Binance / Notion / Google placeholders that are no longer part of the product), along with its test and account-section entry. The deep route `/settings/connections` is gone; references in the navigation hook, Developer Options, and the post-onboarding deep link are removed or repointed to `/settings/composio-routing`. `walletApi` stays — it's still used by the recovery-phrase flow. - Move the Clear-App-Data flow tests to the new component, and add a guard on SettingsHome that the destructive entries no longer appear on the home screen.
Contributor
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 1 minute and 51 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR enhances loopback OAuth callback reliability via Rust task lifecycle tracking and socket reuse, while simultaneously reorganizing the Settings page: moving destructive account actions to a dedicated footer component, consolidating notifications into a hash-driven tabbed interface, and removing the deprecated Connections panel. ChangesLoopback OAuth lifecycle and reliability
Settings page reorganization and account management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
coderabbitai
Bot
added
feature
Contributor
|
test |
Added the two new tab labels (`settings.notifications.tabs.preferences` and `.routing`) to every locale's `*-1.ts` chunk so the i18n coverage gate stops failing on `missing[head]`. Values are English placeholders for non-English locales — they'll show up as untranslated (which is a non-blocking warning) until human translations land.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/utils/loopbackOauthListener.ts (2)
90-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake
cancel()tear down the JS side immediately.Line 90 only invokes the Rust stop command. If
awaitCallback()is already pending, its timeout andlisten()subscription stay alive until callback delivery or the 5-minute timer fires, so a canceled flow can still reject later and keep a stale handler registered. Share the same cleanup path between callback, timeout, and cancel socancel()unlistens, clearsactiveUnlisten, cancels the timer, and rejects the pending promise right away.🤖 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/src/utils/loopbackOauthListener.ts` around lines 90 - 123, The cancel() path currently only calls stop() and doesn't perform the JS-side cleanup used by awaitCallback(), so a pending awaitCallback() stays subscribed and its timer can later reject; modify awaitCallback() to expose a shared teardown function that clears the timeout (timer), calls the current unlisten (and sets activeUnlisten = null), and rejects the pending promise, and then update cancel() to call that teardown before/alongside invoking('stop_loopback_oauth_listener') so cancel immediately unlistens, clears activeUnlisten, clears the timer, and rejects the pending awaitCallback() promise; reference awaitCallback(), cancel(), stop(), activeUnlisten, unlisten, timer, listen, CALLBACK_EVENT to locate spots to wire the shared cleanup.
52-118: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd namespaced diagnostics around listener replacement and cleanup.
This file adds new stateful cleanup paths (
activeUnlistenreplacement, callback consume, timeout) but still has nodebug()tracing for the entry/exit and branch transitions that would explain duplicate-callback regressions. Please instrument these paths with a named logger and avoid logging the callback URL or state value.As per coding guidelines: "Debug logging in the app should be namespaced using
debug()from a named logger instance and include dev-only detail" and "All changes lacking diagnosis logging are incomplete."🤖 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/src/utils/loopbackOauthListener.ts` around lines 52 - 118, Add a namespaced debug logger and sprinkle debug() calls through startLoopbackOauthListener and its inner helpers (notably the activeUnlisten replacement block, the result handling after invoke('start_loopback_oauth_listener'), awaitCallback promise branches, the timeout handler, the listen() resolution, and stop()). Log entry/exit for startLoopbackOauthListener and awaitCallback, and branch events like "listener-created", "listener-replaced", "callback-consumed", "listener-timeout", and "listener-stopped" using the logger; do NOT log the callback URL or the raw state value (log presence or an opaque marker only). Use the same logger instance name across these sites so logs are namespaced and filterable.app/src-tauri/src/loopback_oauth.rs (1)
177-236:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSerialize loopback listener startup across concurrent invokes.
Line 186 cancels only the listener currently stored in
ACTIVE_LISTENER, but the cancel → bind → spawn → install sequence is still open to races. Two near-simultaneousstart_loopback_oauth_listenercalls can both observeNone, bind different sockets (requested port + ephemeral fallback), and then compete to install themselves. The loser can still emitloopback-oauth-callbackinto the latest frontend subscription, completing the wrong OAuth round-trip. Guard the whole start path behind a single async mutex/state machine so only one start can be in flight at a time.🤖 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/src-tauri/src/loopback_oauth.rs` around lines 177 - 236, Concurrent invocations of start_loopback_oauth_listener can race between take_active_listener, bind_loopback, spawn and install_active_listener; protect the entire startup sequence by introducing a global async mutex (e.g., START_STARTUP_MUTEX: tokio::sync::Mutex or tauri::async_runtime::Mutex) and await a lock at the top of start_loopback_oauth_listener before calling take_active_listener, bind_loopback, spawning the task and install_active_listener, then release the lock after install_active_listener completes; ensure the critical section covers the cancel→bind→spawn→install path so only one start is in-flight and references unique symbols start_loopback_oauth_listener, take_active_listener, bind_loopback, install_active_listener and NEXT_LISTENER_ID in the change.
🧹 Nitpick comments (3)
app/src/components/settings/SettingsHome.tsx (1)
1-1: ⚡ Quick winUse
import typeforReactNode.Line 1 should be a type-only import to match repo TS conventions and avoid unnecessary value-import semantics.
-import { ReactNode } from 'react'; +import type { ReactNode } from 'react';As per coding guidelines: "Use static
import typefor TypeScript type imports instead ofimportto enable tree-shaking and faster type checking."🤖 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/src/components/settings/SettingsHome.tsx` at line 1, Change the value import to a type-only import for ReactNode: replace the current import of ReactNode with a TypeScript type import (use the "import type" form) at the top of the file so that ReactNode is imported as a type-only symbol; update the import statement that references ReactNode in SettingsHome.tsx to use "import type { ReactNode } from 'react'".app/src/components/settings/LogoutAndClearActions.tsx (1)
76-77: ⚡ Quick winAdd dialog semantics to the confirmation modal.
The modal should expose dialog semantics (
role="dialog",aria-modal, and label wiring) so assistive tech treats it as a blocking confirmation flow.Also applies to: 94-96
🤖 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/src/components/settings/LogoutAndClearActions.tsx` around lines 76 - 77, The confirmation modal in LogoutAndClearActions lacks dialog semantics; update the modal wrapper divs to include role="dialog" and aria-modal="true", add an aria-labelledby that points to the modal title element and an aria-describedby that points to the explanatory text/confirmation message, and ensure the title and message elements have unique ids so assistive tech can reference them; apply the same changes to the second confirmation modal instance in this component so both modals are properly labelled and announced.app/src/components/settings/__tests__/LogoutAndClearActions.test.tsx (1)
11-16: ⚡ Quick winPrefer shared test helpers over a bespoke render/store harness.
This setup is valid, but it duplicates test plumbing that should come from
app/src/test/helpers to keep settings tests consistent.As per coding guidelines: "Use existing test helpers from
app/src/test/(test-utils.tsx, shared mock backend) before adding new harness code in Vitest tests."Also applies to: 32-40
🤖 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/src/components/settings/__tests__/LogoutAndClearActions.test.tsx` around lines 11 - 16, The test introduces a bespoke store helper (makeTestStore) duplicating shared test plumbing; replace its usage in LogoutAndClearActions.test.tsx with the project's shared test helpers (the test-utils.tsx render/store harness and shared mock backend) instead of calling configureStore directly with localeReducer and preloadedState; locate and remove makeTestStore and update tests to use the common renderWithProviders / getTestStore helper from the shared test utilities so settings tests share the same store/mocks as other tests.
🤖 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/src-tauri/permissions/allow-core-process.toml`:
- Around line 129-137: Remove the loopback OAuth entries from the broad
allow-core-process group and place them into a new, narrowly-scoped permission
TOML (e.g., create a new file that declares "start_loopback_oauth_listener" and
"stop_loopback_oauth_listener" as its permitted commands). Update any capability
grants that currently rely on allow-core-process so the OAuth surface (the
OAuth/login component) is the only one granted the new permission TOML, and
ensure existing consumers that should not get OAuth control keep referencing
allow-core-process unchanged. Verify no other modules still depend on
start_loopback_oauth_listener/stop_loopback_oauth_listener from
allow-core-process and adjust their capability declarations to reference the new
permission.
In `@app/src/components/settings/LogoutAndClearActions.tsx`:
- Around line 24-25: In LogoutAndClearActions (e.g., inside the logout handler
where console.warn('[Account] Rust logout failed:', err) and
setError(t('clearData.failedLogout')) are used), replace the raw console.warn
with a namespaced debug logger (create/import a debug instance such as
debug('app:logout') or similar) and log only a sanitized message or minimal
reason (no full err object or stack) while still calling
setError(t('clearData.failedLogout')); additionally, add proper dialog semantics
to the modal container element by adding role="dialog", aria-modal="true", and
aria-labelledby pointing to the modal title element's id (ensure the title
element has that id) so accessibility attributes are present.
In `@app/src/components/settings/panels/NotificationsTabbedPanel.tsx`:
- Around line 28-38: Replace the effect-driven local state with a derived value:
remove useState and the useEffect that syncs tab with location.hash, and instead
declare tab directly as const tab: TabId = hashToTab(location.hash); update
selectTab (and any callers) to stop calling setTab and only call
navigate(`${location.pathname}${TAB_HASH[next]}`, { replace: true }); retain
hashToTab, selectTab, TAB_HASH, navigate, and the TabId typing so the UI is
driven solely from location.hash.
---
Outside diff comments:
In `@app/src-tauri/src/loopback_oauth.rs`:
- Around line 177-236: Concurrent invocations of start_loopback_oauth_listener
can race between take_active_listener, bind_loopback, spawn and
install_active_listener; protect the entire startup sequence by introducing a
global async mutex (e.g., START_STARTUP_MUTEX: tokio::sync::Mutex or
tauri::async_runtime::Mutex) and await a lock at the top of
start_loopback_oauth_listener before calling take_active_listener,
bind_loopback, spawning the task and install_active_listener, then release the
lock after install_active_listener completes; ensure the critical section covers
the cancel→bind→spawn→install path so only one start is in-flight and references
unique symbols start_loopback_oauth_listener, take_active_listener,
bind_loopback, install_active_listener and NEXT_LISTENER_ID in the change.
In `@app/src/utils/loopbackOauthListener.ts`:
- Around line 90-123: The cancel() path currently only calls stop() and doesn't
perform the JS-side cleanup used by awaitCallback(), so a pending
awaitCallback() stays subscribed and its timer can later reject; modify
awaitCallback() to expose a shared teardown function that clears the timeout
(timer), calls the current unlisten (and sets activeUnlisten = null), and
rejects the pending promise, and then update cancel() to call that teardown
before/alongside invoking('stop_loopback_oauth_listener') so cancel immediately
unlistens, clears activeUnlisten, clears the timer, and rejects the pending
awaitCallback() promise; reference awaitCallback(), cancel(), stop(),
activeUnlisten, unlisten, timer, listen, CALLBACK_EVENT to locate spots to wire
the shared cleanup.
- Around line 52-118: Add a namespaced debug logger and sprinkle debug() calls
through startLoopbackOauthListener and its inner helpers (notably the
activeUnlisten replacement block, the result handling after
invoke('start_loopback_oauth_listener'), awaitCallback promise branches, the
timeout handler, the listen() resolution, and stop()). Log entry/exit for
startLoopbackOauthListener and awaitCallback, and branch events like
"listener-created", "listener-replaced", "callback-consumed",
"listener-timeout", and "listener-stopped" using the logger; do NOT log the
callback URL or the raw state value (log presence or an opaque marker only). Use
the same logger instance name across these sites so logs are namespaced and
filterable.
---
Nitpick comments:
In `@app/src/components/settings/__tests__/LogoutAndClearActions.test.tsx`:
- Around line 11-16: The test introduces a bespoke store helper (makeTestStore)
duplicating shared test plumbing; replace its usage in
LogoutAndClearActions.test.tsx with the project's shared test helpers (the
test-utils.tsx render/store harness and shared mock backend) instead of calling
configureStore directly with localeReducer and preloadedState; locate and remove
makeTestStore and update tests to use the common renderWithProviders /
getTestStore helper from the shared test utilities so settings tests share the
same store/mocks as other tests.
In `@app/src/components/settings/LogoutAndClearActions.tsx`:
- Around line 76-77: The confirmation modal in LogoutAndClearActions lacks
dialog semantics; update the modal wrapper divs to include role="dialog" and
aria-modal="true", add an aria-labelledby that points to the modal title element
and an aria-describedby that points to the explanatory text/confirmation
message, and ensure the title and message elements have unique ids so assistive
tech can reference them; apply the same changes to the second confirmation modal
instance in this component so both modals are properly labelled and announced.
In `@app/src/components/settings/SettingsHome.tsx`:
- Line 1: Change the value import to a type-only import for ReactNode: replace
the current import of ReactNode with a TypeScript type import (use the "import
type" form) at the top of the file so that ReactNode is imported as a type-only
symbol; update the import statement that references ReactNode in
SettingsHome.tsx to use "import type { ReactNode } from 'react'".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd1ecda2-871c-4307-8e93-0aa596a251e8
📒 Files selected for processing (20)
app/src-tauri/permissions/allow-core-process.tomlapp/src-tauri/src/loopback_oauth.rsapp/src/components/oauth/OAuthProviderButton.tsxapp/src/components/settings/LogoutAndClearActions.tsxapp/src/components/settings/SettingsHome.tsxapp/src/components/settings/SettingsSectionPage.tsxapp/src/components/settings/__tests__/LogoutAndClearActions.test.tsxapp/src/components/settings/__tests__/SettingsHome.test.tsxapp/src/components/settings/hooks/useSettingsNavigation.tsapp/src/components/settings/panels/ConnectionsPanel.tsxapp/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/components/settings/panels/NotificationRoutingPanel.tsxapp/src/components/settings/panels/NotificationsPanel.tsxapp/src/components/settings/panels/NotificationsTabbedPanel.tsxapp/src/components/settings/panels/__tests__/ConnectionsPanel.test.tsxapp/src/lib/i18n/en.tsapp/src/pages/Home.tsxapp/src/pages/Settings.tsxapp/src/pages/onboarding/customWizardSteps.tsapp/src/utils/loopbackOauthListener.ts
💤 Files with no reviewable changes (3)
- app/src/components/settings/panels/ConnectionsPanel.tsx
- app/src/components/settings/panels/tests/ConnectionsPanel.test.tsx
- app/src/components/settings/hooks/useSettingsNavigation.ts
- Split loopback OAuth into its own `allow-loopback-oauth` permission TOML
so consumers of the broad `allow-core-process` capability don't inherit
control of the OAuth listener. Add the new permission to the default
desktop capability and drop the entries from `allow-core-process`.
- LogoutAndClearActions: replace raw console.warn with a namespaced
`debug('settings:account:warn')` logger, sanitize the error before
logging (message only, no stack / raw payload), and tag the confirm
modal with `role="dialog"`, `aria-modal`, and `aria-labelledby`.
- NotificationsTabbedPanel: drop the useState + useEffect mirror and
derive `tab` directly from `location.hash`. The router is already the
source of truth — the local state was just churn.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/settings/__tests__/SettingsHome.test.tsx (1)
59-73: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPrefer
app/src/test/helpers over local test harness setup.This custom render/store wrapper should be replaced with the shared test utilities used across the repo to keep test setup consistent.
As per coding guidelines: “Use existing test helpers from
app/src/test/(test-utils.tsx, shared mock backend) before adding new harness code in Vitest tests.”🤖 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/src/components/settings/__tests__/SettingsHome.test.tsx` around lines 59 - 73, Replace the local renderSettingsHome harness with the repo's shared test helpers: remove the custom makeTestStore/Provider/MemoryRouter/I18nProvider wrapping in renderSettingsHome and instead call the shared render helper (e.g., renderWithProviders or renderWithI18n) that accepts a locale option and handles store/router/i18n setup; update the test imports to use that helper and adjust any calls to renderSettingsHome to pass locale/withI18n through the shared helper API, then delete the now-unused renderSettingsHome function and its related local test-only setup.
🧹 Nitpick comments (1)
app/src/utils/loopbackOauthListener.ts (1)
1-4: 💤 Low valueConsider separating type-only imports.
The
UnlistenFntype is imported inline with thelistenvalue. As per coding guidelines, prefer a separateimport typestatement for type-only imports to enable tree-shaking and faster type checking.♻️ Suggested fix
import { invoke } from '`@tauri-apps/api/core`'; -import { listen, type UnlistenFn } from '`@tauri-apps/api/event`'; +import { listen } from '`@tauri-apps/api/event`'; +import type { UnlistenFn } from '`@tauri-apps/api/event`';🤖 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/src/utils/loopbackOauthListener.ts` around lines 1 - 4, The import mixes a value import (listen) with a type-only import (UnlistenFn); change the statement to import listen as a regular import and move UnlistenFn to a separate `import type { UnlistenFn } from '`@tauri-apps/api/event`'` so the type is erased at runtime and enables better tree-shaking/type-checking; update the imports where `listen` and `UnlistenFn` are referenced (e.g., in loopbackOauthListener.ts) accordingly.
🤖 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/src/components/settings/__tests__/LogoutAndClearActions.test.tsx`:
- Around line 11-16: Replace the bespoke store/render harness (the makeTestStore
function and any local render wrapper used in this test file) with the shared
test helpers from app/src/test/, e.g., import and use the test-utils exports
such as renderWithProviders or render (and the shared mock backend helper)
instead of calling makeTestStore or configuring a local store; update the test
setup in LogoutAndClearActions.test.tsx to remove makeTestStore and use the
shared helpers for preloaded locale state (set current: 'en') and mock backend
wiring used by other suites (also apply the same replacement for the logic
around lines 32–40).
In `@app/src/components/settings/LogoutAndClearActions.tsx`:
- Around line 24-33: handleLogout currently calls setError(...) on failure but
that error state is only visible inside the clear-data modal which is not shown
on logout failure; update handleLogout (and the similar block at 125-129) to
both set the error and ensure the clear-data modal is displayed (e.g., call the
existing state setter like setShowClearDataModal(true) or invoke the app's
global notification/toast function) so users see the failure; reference
handleLogout, clearSession, setError and the modal visibility setter (or global
toast API) when making the change.
In `@app/src/components/settings/SettingsHome.tsx`:
- Line 1: Change the import of ReactNode in SettingsHome.tsx to a type-only
import: replace the regular import that references ReactNode with an import type
{ ReactNode } so the symbol ReactNode is imported purely as a TypeScript type
(ensuring tree-shaking and faster type checking).
---
Outside diff comments:
In `@app/src/components/settings/__tests__/SettingsHome.test.tsx`:
- Around line 59-73: Replace the local renderSettingsHome harness with the
repo's shared test helpers: remove the custom
makeTestStore/Provider/MemoryRouter/I18nProvider wrapping in renderSettingsHome
and instead call the shared render helper (e.g., renderWithProviders or
renderWithI18n) that accepts a locale option and handles store/router/i18n
setup; update the test imports to use that helper and adjust any calls to
renderSettingsHome to pass locale/withI18n through the shared helper API, then
delete the now-unused renderSettingsHome function and its related local
test-only setup.
---
Nitpick comments:
In `@app/src/utils/loopbackOauthListener.ts`:
- Around line 1-4: The import mixes a value import (listen) with a type-only
import (UnlistenFn); change the statement to import listen as a regular import
and move UnlistenFn to a separate `import type { UnlistenFn } from
'`@tauri-apps/api/event`'` so the type is erased at runtime and enables better
tree-shaking/type-checking; update the imports where `listen` and `UnlistenFn`
are referenced (e.g., in loopbackOauthListener.ts) accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d1b9be6-465d-4b5c-8061-4b795fd72cfa
📒 Files selected for processing (34)
app/src-tauri/capabilities/default.jsonapp/src-tauri/permissions/allow-loopback-oauth.tomlapp/src-tauri/src/loopback_oauth.rsapp/src/components/oauth/OAuthProviderButton.tsxapp/src/components/settings/LogoutAndClearActions.tsxapp/src/components/settings/SettingsHome.tsxapp/src/components/settings/SettingsSectionPage.tsxapp/src/components/settings/__tests__/LogoutAndClearActions.test.tsxapp/src/components/settings/__tests__/SettingsHome.test.tsxapp/src/components/settings/hooks/useSettingsNavigation.tsapp/src/components/settings/panels/ConnectionsPanel.tsxapp/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/components/settings/panels/NotificationRoutingPanel.tsxapp/src/components/settings/panels/NotificationsPanel.tsxapp/src/components/settings/panels/NotificationsTabbedPanel.tsxapp/src/components/settings/panels/__tests__/ConnectionsPanel.test.tsxapp/src/lib/i18n/chunks/ar-1.tsapp/src/lib/i18n/chunks/bn-1.tsapp/src/lib/i18n/chunks/de-1.tsapp/src/lib/i18n/chunks/en-1.tsapp/src/lib/i18n/chunks/es-1.tsapp/src/lib/i18n/chunks/fr-1.tsapp/src/lib/i18n/chunks/hi-1.tsapp/src/lib/i18n/chunks/id-1.tsapp/src/lib/i18n/chunks/it-1.tsapp/src/lib/i18n/chunks/ko-1.tsapp/src/lib/i18n/chunks/pt-1.tsapp/src/lib/i18n/chunks/ru-1.tsapp/src/lib/i18n/chunks/zh-CN-1.tsapp/src/lib/i18n/en.tsapp/src/pages/Home.tsxapp/src/pages/Settings.tsxapp/src/pages/onboarding/customWizardSteps.tsapp/src/utils/loopbackOauthListener.ts
💤 Files with no reviewable changes (3)
- app/src/components/settings/panels/ConnectionsPanel.tsx
- app/src/components/settings/panels/tests/ConnectionsPanel.test.tsx
- app/src/components/settings/hooks/useSettingsNavigation.ts
Round-2 CodeRabbit fixes on PR tinyhumansai#2550: - Render an inline `role="alert"` banner below the Log out row when `handleLogout` fails. Previously `setError` was only displayed inside the clear-data modal, so a failed logout was silent unless the user happened to open that modal afterwards. - Add a failure-path test covering the inline error path. - Migrate LogoutAndClearActions tests to the shared `renderWithProviders` helper from `app/src/test/test-utils.tsx` instead of a bespoke `makeTestStore`/wrapper, matching project guidelines. - SettingsHome: use `import type { ReactNode }` since the symbol is only used as a type.
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
May 24, 2026
Post PR tinyhumansai#2550 real OAuth flows hit a loopback HTTP server (RFC 8252) and the openhuman:// deep link is only the fallback. The E2E auth bypass was still firing openhuman://auth?token=... directly through window.__simulateDeepLink, the legacy path. Switch it to loopback so every spec exercises the same Rust HTTP server + state-nonce check + Tauri event emit that ships to users. - app/src/utils/loopbackOauthListener.ts: expose startLoopbackOauthListener on window.__startLoopbackOauthListener when the E2E build flag (VITE_OPENHUMAN_E2E_RESTART_APP_AS_RELOAD) is set. No prod-bundle leak. - app/test/e2e/helpers/loopback-auth-helpers.ts: new triggerAuthLoopbackBypass(userId). WebView starts the production loopback listener and wires its awaitCallback() to __simulateDeepLink the same way OAuthProviderButton does. Node then fetches the loopback URL with the bypass JWT + matching state nonce appended; the Rust listener accepts, validates, and emits loopback-oauth-callback. The WebView listener rewrites the URL to openhuman://auth?... and routes it through the existing handleDeepLinkUrls pipeline. - app/test/e2e/helpers/reset-app.ts: use the new helper in both the primary and retry auth bootstrap calls. resetApp's external contract is unchanged; specs see the same authenticated state at the end. Non-auth deep-link helpers stay for mega-flow's oauth-success integration callbacks, which still fire through the openhuman:// fallback path.
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
May 24, 2026
…ucture PR tinyhumansai#2550 (oauth loopback + settings cleanup) moved several settings surfaces. Tests were still hitting the old routes/text and producing 13 Linux full-suite failures across foundation / chat / commerce shards. Fix the four most-affected: - shared-flows.ts logoutViaSettings(): logout + clear-data buttons moved out of /settings (main page) and into the Account section (LogoutAndClearActions footer on /settings/account). Navigate straight there. Unblocks auth-access-control, logout-relogin-onboarding, runtime-picker-login, onboarding-modes (all of which failed with 'Could not find logout button'). - settings-data-management.spec.ts: same root cause — Clear App Data lives at /settings/account now, not /settings. - settings-account-preferences.spec.ts: /settings/connections (the Web3 Wallet status card) was deleted in PR tinyhumansai#2550. Drop the navigation + waitForText. The openhuman.wallet_status RPC assertion above is the canonical signal that the recovery-phrase flow wired through. - settings-advanced-config.spec.ts: 'Notification Routing' was removed as a top-level Developer Options entry (now a tab on the Notifications panel). Drop the waitForText. The persistence test navigates to /settings/notifications and clicks the Routing tab instead of the legacy /settings/notification-routing path (HashRouter doesn't carry through the redirect cleanly for navigateViaHash's hash-match check).
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
May 24, 2026
Same root cause as the other settings fixes — PR tinyhumansai#2550 moved Log out out of the main /settings page into the Account section. This spec has its own inline logout sequence (doesn't use logoutViaSettings) so it needed the route update separately. 9 of 9 tests passing locally after the change.
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
May 24, 2026
- screen-intelligence: 'Permissions' sub-section is conditionally rendered only when status.platform_supported is true (macOS). ScreenIntelligencePanel:121 gates it out on Linux/Windows so the assertion has always been impossible to satisfy in CI. Keep the 'Screen Awareness' title check (renders everywhere) and drop the Permissions one — the second test in this spec already exercises the debug panel where permissions UI is reachable for diagnostics. - navigation-settings-panels N2.2: /settings/connections route was deleted in PR tinyhumansai#2550 (ConnectionsPanel removed). Mark the case .skip with a pointer to the PR so test numbering N2.1..N2.9 still reads consistently against the spec list.
7 tasks
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
2 participants
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
Problem
The loopback OAuth path landed in #2511 but was effectively dead on arrival:
StartResultserialized asredirect_uri/statewhile the TS wrapper readresult.redirectUri. The undefined value tripped a silent TypeError inappendState, so every successful Rust-side bind was immediately abandoned in JS. Net effect: every OAuth click silently fell back to theopenhuman://deep link.pnpm dev:app), the OAuth button always setresponseType=jsoneven when a loopback was active. The backend then returned JSON in the browser tab instead of redirecting, so the loopback listener never fired.start_loopback_oauth_listener/stop_loopback_oauth_listenerTauri commands weren't in theallow-core-processcapability allow-list, so every invoke returned "Command not found".EADDRINUSE: cancel was just a oneshot signal, not awaited.listen()handler from a previous click stayed registered after Rust cancelled its listener, so the next callback fanned out to multiple stale handlers.The Settings home was also getting noisy — destructive actions mixed with navigation rows, and notification preferences vs routing lived in two unrelated entries. The Connections panel exposed Web3 / wallet placeholders that aren't part of the product anymore.
Solution
StartResultwith#[serde(rename_all = \"camelCase\")].responseType=jsonwhen there is no loopback handle (web build or bind failure).start_loopback_oauth_listener/stop_loopback_oauth_listenertoallow-core-process.toml.TcpSocketwithSO_REUSEADDR, hold aJoinHandlefor the active listener, andawaitit before re-binding so the OS has actually released the fixed loopback port.redirectUriwhitelist restricts host but not port.listen()handle before installing a new one.LogoutAndClearActionsand render it as the footer of the Account page via a newfooterprop onSettingsSectionPage.NotificationsTabbedPanelowns the header + tab strip and embeds both existing panels (each gained anembeddedflag to suppress its own chrome). Legacy/settings/notification-routingredirects to/settings/notifications#routing.ConnectionsPanel.tsx+ its test; remove the account-section entry, route, navigation hook type, and Developer Options entry. Repoint the post-onboardingoauthwizard deep link to/settings/composio-routing.walletApistays — it's still used by the recovery-phrase flow.The Settings → Account page now renders Recovery Phrase / Team / Privacy / Migration followed by Clear App Data + Log out. The home menu is purely navigational.
Submission Checklist
Impact
openhuman://.redirectUriquery param./settings/connectionsis gone.Pre-push hook note: the pre-push hook auto-applied formatter fixes (Prettier import-order + cargo fmt). Committed as
chore: apply auto-fixes. No--no-verifywas used.Related
loopback_oauthunit tests).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
Validation Blocked
Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
UI Changes
Tests
Refactor