fix(onboarding): make Connect your apps toggles start auth and reflect real connection state#1226
Open
graycyrus wants to merge 4 commits intotinyhumansai:mainfrom
Open
fix(onboarding): make Connect your apps toggles start auth and reflect real connection state#1226graycyrus wants to merge 4 commits intotinyhumansai:mainfrom
graycyrus wants to merge 4 commits intotinyhumansai:mainfrom
Conversation
…t real connection state When the user toggles ON a provider and clicks Done, the modal now navigates to /chat and activates the first new account so its WebviewHost mounts and the auth webview opens immediately. Toggles also show live status indicators (Connected / Needs sign-in / Error) instead of just on/off. Closes tinyhumansai#1154
Contributor
📝 WalkthroughWalkthroughThe PR enhances the accounts/setup flow in OpenhumanLinkModal: toggling a provider creates/removes local webview accounts and shows lifecycle statuses; Done closes the modal, activates the first newly-added account, and navigates to /chat to start the auth/webview flow. Tests for the full flow are added. ChangesAccounts Setup Integration and Navigation
Sequence DiagramsequenceDiagram
participant User
participant Modal as OpenhumanLinkModal
participant Redux as Redux Store
participant Router as React Router
participant Auth as Webview/Auth Flow
User->>Modal: Toggle provider ON
Modal->>Redux: Dispatch create account (status: pending)
Redux-->>Modal: Account present in store
Modal->>Modal: Add id to newlyAdded
User->>Modal: Toggle provider OFF
Modal->>Redux: Dispatch remove account
Redux-->>Modal: Account removed
Modal->>Modal: Remove id from newlyAdded
User->>Modal: Click "Done"
Modal->>Redux: Dispatch setActiveAccount(first newlyAdded)
Redux-->>Modal: activeAccountId updated
Modal->>Router: navigate('/chat')
Router->>Auth: Start webview/auth flow
Modal->>Modal: Close modal
User->>Modal: Click "Skip for now"
Modal->>Modal: Close modal (no navigation)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
app/src/components/OpenhumanLinkModal.tsx (1)
443-450:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe row state is only looking at one account per provider.
This flow now claims to show the real connection state, but
handleToggleand the new status UI both operate on just the first account returned for a provider. Since the store allows multiple accounts with the sameprovider, the row can show the wrong status and a single "Disconnect" click can remove one account while leaving the toggle effectively on because another same-provider account still exists. Consider deriving the row from all accounts for that provider and making disconnect semantics explicit.Also applies to: 483-498
🤖 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/OpenhumanLinkModal.tsx` around lines 443 - 450, The row currently checks only a single account via accountByProvider.get and handleToggle removes just that one id; change the logic to derive row state from all accounts for a provider (e.g., compute accountsForProvider = accounts.filter(a => a.provider === providerId) and isConnected = accountsForProvider.length > 0), update the UI to use isConnected instead of a single account, and make disconnect semantics explicit in handleToggle (either removeAll: iterate accountsForProvider and call purgeWebviewAccount + dispatch(removeAccount) for each, or accept an accountId param to remove a specific account). Replace usages of accountByProvider.get(providerId) in handleToggle and the corresponding status UI with the aggregated accountsForProvider logic and update newlyAdded handling to operate on each removed id.
🧹 Nitpick comments (1)
app/src/components/OpenhumanLinkModal.tsx (1)
26-31: ⚡ Quick winSplit the type-only imports into an
import typestatement.
Account,AccountProvider, andAccountStatusare type-only here, so they should be imported via a separateimport typeline instead of a mixed value import withPROVIDERS.As per coding guidelines,
**/*.{ts,tsx}: Useimport typefor type-only imports in TypeScript.🤖 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/OpenhumanLinkModal.tsx` around lines 26 - 31, The current import mixes type-only symbols with a runtime value; split the types into an explicit type-only import: import type { Account, AccountProvider, AccountStatus } from '...'; and keep PROVIDERS in a separate value import: import { PROVIDERS } from '...'; update the existing import that currently contains Account, AccountProvider, AccountStatus, and PROVIDERS so that only PROVIDERS remains in the runtime import and the three types are moved to an import type line to satisfy the TypeScript guideline.
🤖 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 `@app/src/components/OpenhumanLinkModal.tsx`:
- Around line 443-450: The row currently checks only a single account via
accountByProvider.get and handleToggle removes just that one id; change the
logic to derive row state from all accounts for a provider (e.g., compute
accountsForProvider = accounts.filter(a => a.provider === providerId) and
isConnected = accountsForProvider.length > 0), update the UI to use isConnected
instead of a single account, and make disconnect semantics explicit in
handleToggle (either removeAll: iterate accountsForProvider and call
purgeWebviewAccount + dispatch(removeAccount) for each, or accept an accountId
param to remove a specific account). Replace usages of
accountByProvider.get(providerId) in handleToggle and the corresponding status
UI with the aggregated accountsForProvider logic and update newlyAdded handling
to operate on each removed id.
---
Nitpick comments:
In `@app/src/components/OpenhumanLinkModal.tsx`:
- Around line 26-31: The current import mixes type-only symbols with a runtime
value; split the types into an explicit type-only import: import type { Account,
AccountProvider, AccountStatus } from '...'; and keep PROVIDERS in a separate
value import: import { PROVIDERS } from '...'; update the existing import that
currently contains Account, AccountProvider, AccountStatus, and PROVIDERS so
that only PROVIDERS remains in the runtime import and the three types are moved
to an import type line to satisfy the TypeScript guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69926bea-67d1-4006-ae6b-fe91d6991fa1
📒 Files selected for processing (2)
app/src/components/OpenhumanLinkModal.tsxapp/src/components/__tests__/OpenhumanLinkModal.accounts.test.tsx
Contributor
Author
CodeRabbit review responsesNitpick (import type split): Fixed in 12849c3 — split type-only imports into a separate Major (single account per provider): This is by design and matches the pre-existing behavior. The |
Done button now shows "Continue with <app> sign-in" when a provider is toggled on, making it clear what happens next.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/components/__tests__/OpenhumanLinkModal.accounts.test.tsx (1)
152-164: 💤 Low valueConsider using the imported action creator for type safety.
Dispatching with a raw type string (
'accounts/addAccount') works but bypasses TypeScript's payload type checking. Using theaddAccountaction creator would catch payload shape mismatches at compile time.♻️ Suggested change
+import { addAccount } from '../../store/accountsSlice'; + it('shows status indicator for existing accounts with a status', () => { const store = createStore(); - store.dispatch({ - type: 'accounts/addAccount', - payload: { - id: 'test-acct-1', - provider: 'telegram', - label: 'Telegram', - createdAt: new Date().toISOString(), - status: 'open', - }, - }); + store.dispatch( + addAccount({ + id: 'test-acct-1', + provider: 'telegram', + label: 'Telegram', + createdAt: new Date().toISOString(), + status: 'open', + }) + );🤖 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/__tests__/OpenhumanLinkModal.accounts.test.tsx` around lines 152 - 164, Replace the raw action dispatch that uses the string type with the typed action creator to get TypeScript payload checking: import and use the addAccount action creator (e.g., call store.dispatch(addAccount({...}))) instead of dispatching { type: 'accounts/addAccount', payload: ... } in the test that sets up the account (the block using createStore and dispatch), keeping the same payload fields but relying on addAccount for correct typing.
🤖 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/OpenhumanLinkModal.tsx`:
- Around line 398-414: The statusDisplay function can return undefined if
AccountStatus gains new values; add an exhaustive guard so it always returns the
expected {label,dotClass}. Update statusDisplay to include a default/fallback
branch that either throws a descriptive Error (e.g., `Unknown AccountStatus:
${status}`) or calls an assertUnreachable/never helper to force compile-time
exhaustiveness, ensuring the function always returns the correct shape for any
AccountStatus value.
---
Nitpick comments:
In `@app/src/components/__tests__/OpenhumanLinkModal.accounts.test.tsx`:
- Around line 152-164: Replace the raw action dispatch that uses the string type
with the typed action creator to get TypeScript payload checking: import and use
the addAccount action creator (e.g., call store.dispatch(addAccount({...})))
instead of dispatching { type: 'accounts/addAccount', payload: ... } in the test
that sets up the account (the block using createStore and dispatch), keeping the
same payload fields but relying on addAccount for correct typing.
🪄 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: bef564a9-28cc-4577-85a9-a28da64cb789
📒 Files selected for processing (2)
app/src/components/OpenhumanLinkModal.tsxapp/src/components/__tests__/OpenhumanLinkModal.accounts.test.tsx
Comment on lines
+398
to
+414
| /** Status label + color for a given account lifecycle status. */ | ||
| function statusDisplay(status: AccountStatus): { label: string; dotClass: string } { | ||
| switch (status) { | ||
| case 'open': | ||
| return { label: 'Connected', dotClass: 'bg-emerald-500' }; | ||
| case 'loading': | ||
| return { label: 'Loading…', dotClass: 'bg-amber-400' }; | ||
| case 'pending': | ||
| return { label: 'Needs sign-in', dotClass: 'bg-amber-400' }; | ||
| case 'timeout': | ||
| return { label: 'Timed out', dotClass: 'bg-red-400' }; | ||
| case 'error': | ||
| return { label: 'Error', dotClass: 'bg-red-400' }; | ||
| case 'closed': | ||
| return { label: 'Closed', dotClass: 'bg-stone-300' }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add exhaustive check for statusDisplay to prevent runtime errors.
The switch statement lacks a default case. If AccountStatus is extended with a new value and this function isn't updated, it will return undefined, causing the UI to fail when accessing .label or .dotClass.
🛡️ Proposed fix to ensure exhaustiveness
function statusDisplay(status: AccountStatus): { label: string; dotClass: string } {
switch (status) {
case 'open':
return { label: 'Connected', dotClass: 'bg-emerald-500' };
case 'loading':
return { label: 'Loading…', dotClass: 'bg-amber-400' };
case 'pending':
return { label: 'Needs sign-in', dotClass: 'bg-amber-400' };
case 'timeout':
return { label: 'Timed out', dotClass: 'bg-red-400' };
case 'error':
return { label: 'Error', dotClass: 'bg-red-400' };
case 'closed':
return { label: 'Closed', dotClass: 'bg-stone-300' };
+ default: {
+ const _exhaustive: never = status;
+ return { label: 'Unknown', dotClass: 'bg-stone-300' };
+ }
}
}📝 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.
Suggested change
| /** Status label + color for a given account lifecycle status. */ | |
| function statusDisplay(status: AccountStatus): { label: string; dotClass: string } { | |
| switch (status) { | |
| case 'open': | |
| return { label: 'Connected', dotClass: 'bg-emerald-500' }; | |
| case 'loading': | |
| return { label: 'Loading…', dotClass: 'bg-amber-400' }; | |
| case 'pending': | |
| return { label: 'Needs sign-in', dotClass: 'bg-amber-400' }; | |
| case 'timeout': | |
| return { label: 'Timed out', dotClass: 'bg-red-400' }; | |
| case 'error': | |
| return { label: 'Error', dotClass: 'bg-red-400' }; | |
| case 'closed': | |
| return { label: 'Closed', dotClass: 'bg-stone-300' }; | |
| } | |
| } | |
| /** Status label + color for a given account lifecycle status. */ | |
| function statusDisplay(status: AccountStatus): { label: string; dotClass: string } { | |
| switch (status) { | |
| case 'open': | |
| return { label: 'Connected', dotClass: 'bg-emerald-500' }; | |
| case 'loading': | |
| return { label: 'Loading…', dotClass: 'bg-amber-400' }; | |
| case 'pending': | |
| return { label: 'Needs sign-in', dotClass: 'bg-amber-400' }; | |
| case 'timeout': | |
| return { label: 'Timed out', dotClass: 'bg-red-400' }; | |
| case 'error': | |
| return { label: 'Error', dotClass: 'bg-red-400' }; | |
| case 'closed': | |
| return { label: 'Closed', dotClass: 'bg-stone-300' }; | |
| default: { | |
| const _exhaustive: never = status; | |
| return { label: 'Unknown', dotClass: 'bg-stone-300' }; | |
| } | |
| } | |
| } |
🤖 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/OpenhumanLinkModal.tsx` around lines 398 - 414, The
statusDisplay function can return undefined if AccountStatus gains new values;
add an exhaustive guard so it always returns the expected {label,dotClass}.
Update statusDisplay to include a default/fallback branch that either throws a
descriptive Error (e.g., `Unknown AccountStatus: ${status}`) or calls an
assertUnreachable/never helper to force compile-time exhaustiveness, ensuring
the function always returns the correct shape for any AccountStatus value.
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
/chatand activates the first newly-added account, mountingWebviewHostso the auth webview opens immediately (user authenticates in-webview)DoneFooternow accepts an optionalonDoneprop for custom Done behaviorAcceptance criteria (from #1154)
detect_webview_logins())Test plan
pnpm typecheck— passpnpm lint— 0 errorspnpm format:check— passpnpm build— passCloses #1154
Summary by CodeRabbit
New Features
Bug Fixes
Tests