Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/components/ConsoleOAuthFlow.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import { stripVTControlCharacters as stripAnsi } from 'node:util'
import { AppStateProvider } from '../state/AppState.js'
import { createRoot } from '../ink.js'
import { KeybindingSetup } from '../keybindings/KeybindingProviderSetup.js'
import { ConsoleOAuthFlow } from './ConsoleOAuthFlow.js'
import {
ConsoleOAuthFlow,
isSuccessfulProviderSetupResult,
} from './ConsoleOAuthFlow.js'

const SYNC_START = '\x1B[?2026h'
const SYNC_END = '\x1B[?2026l'
Expand Down Expand Up @@ -124,6 +127,28 @@ test('login picker shows the third-party platform option', async () => {
expect(output).toContain('3rd-party platform')
})

test('first-run provider setup accepts both saved profiles and Claude Code activation', () => {
expect(
isSuccessfulProviderSetupResult({
action: 'saved',
message: 'Saved Gitlawb Opengateway',
}),
).toBe(true)
expect(
isSuccessfulProviderSetupResult({
action: 'activated',
message: 'Provider switched to Claude Code (OAuth) (claude-opus-5)',
}),
).toBe(true)
expect(
isSuccessfulProviderSetupResult({
action: 'cancelled',
message: 'Provider setup skipped',
}),
).toBe(false)
expect(isSuccessfulProviderSetupResult(undefined)).toBe(false)
})
Comment on lines +130 to +150

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the message branches of the predicate.

The tests exercise the action check but not the message check. { action: 'saved' } with a missing message and with an empty-string message both reach the platform_setup_complete state in the old code and are exactly what the new helper rejects. Add both cases.

💚 Proposed additions
   expect(isSuccessfulProviderSetupResult(undefined)).toBe(false)
+  expect(isSuccessfulProviderSetupResult({ action: 'saved' })).toBe(false)
+  expect(
+    isSuccessfulProviderSetupResult({ action: 'activated', message: '' }),
+  ).toBe(false)
 })

Validate with bun test ./src/components/ConsoleOAuthFlow.test.tsx. As per path instructions: block when "tests assert implementation details while missing the user-visible behavior"; here the message guard drives whether the success screen renders.

📝 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
test('first-run provider setup accepts both saved profiles and Claude Code activation', () => {
expect(
isSuccessfulProviderSetupResult({
action: 'saved',
message: 'Saved Gitlawb Opengateway',
}),
).toBe(true)
expect(
isSuccessfulProviderSetupResult({
action: 'activated',
message: 'Provider switched to Claude Code (OAuth) (claude-opus-5)',
}),
).toBe(true)
expect(
isSuccessfulProviderSetupResult({
action: 'cancelled',
message: 'Provider setup skipped',
}),
).toBe(false)
expect(isSuccessfulProviderSetupResult(undefined)).toBe(false)
})
test('first-run provider setup accepts both saved profiles and Claude Code activation', () => {
expect(
isSuccessfulProviderSetupResult({
action: 'saved',
message: 'Saved Gitlawb Opengateway',
}),
).toBe(true)
expect(
isSuccessfulProviderSetupResult({
action: 'activated',
message: 'Provider switched to Claude Code (OAuth) (claude-opus-5)',
}),
).toBe(true)
expect(
isSuccessfulProviderSetupResult({
action: 'cancelled',
message: 'Provider setup skipped',
}),
).toBe(false)
expect(isSuccessfulProviderSetupResult(undefined)).toBe(false)
expect(isSuccessfulProviderSetupResult({ action: 'saved' })).toBe(false)
expect(
isSuccessfulProviderSetupResult({ action: 'activated', message: '' }),
).toBe(false)
})
🤖 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 `@src/components/ConsoleOAuthFlow.test.tsx` around lines 130 - 150, Extend the
test for isSuccessfulProviderSetupResult to cover saved results with a missing
message and with an empty-string message, asserting both are rejected. Keep the
existing valid saved, activated, cancelled, and undefined cases, and run the
targeted ConsoleOAuthFlow test.

Source: Path instructions


test('third-party provider branch opens the first-run provider manager', async () => {
const output = await renderFrame(
<ConsoleOAuthFlow
Expand Down
19 changes: 17 additions & 2 deletions src/components/ConsoleOAuthFlow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,26 @@ import {
updateProviderProfile,
} from '../utils/providerProfiles.js';
import { getSettings_DEPRECATED } from '../utils/settings/settings.js';
import { ProviderManager } from './ProviderManager.js';
import {
ProviderManager,
type ProviderManagerResult,
} from './ProviderManager.js';
import { Select } from './CustomSelect/select.js';
import { KeyboardShortcutHint } from './design-system/KeyboardShortcutHint.js';
import { Spinner } from './Spinner.js';
import TextInput from './TextInput.js';

export function isSuccessfulProviderSetupResult(
result: ProviderManagerResult | undefined,
): boolean {
return (
!!result &&
(result.action === 'saved' || result.action === 'activated') &&
typeof result.message === 'string' &&
result.message.length > 0
)
}
Comment on lines +33 to +42

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -nP -A6 "platform_setup_complete" src/components/ConsoleOAuthFlow.tsx | head -60
rg -nP -B2 -A12 "type OAuthStatus" src/components/ConsoleOAuthFlow.tsx

Repository: Gitlawb/openclaude

Length of output: 2546


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- ProviderManagerResult definitions and usages ---'
rg -n -C 5 "ProviderManagerResult" src

printf '%s\n' '--- Helper and call-site context ---'
sed -n '1,90p' src/components/ConsoleOAuthFlow.tsx
sed -n '510,600p' src/components/ConsoleOAuthFlow.tsx

printf '%s\n' '--- TypeScript configuration and scripts ---'
rg -n -C 3 '"typecheck|typecheck:type-tests"' package.json
fd -a 'tsconfig*.json' . -x sh -c 'echo "--- $1"; sed -n "1,180p" "$1"' sh {}

Repository: Gitlawb/openclaude

Length of output: 12430


Return a type predicate from isSuccessfulProviderSetupResult.

OAuthStatus requires message: string for platform_setup_complete, but ProviderManagerResult.message is optional. A boolean return does not narrow result.message, so strict TypeScript rejects this state transition. Use result is ProviderManagerResult & { message: 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 `@src/components/ConsoleOAuthFlow.tsx` around lines 33 - 42, Update
isSuccessfulProviderSetupResult to return the type predicate result is
ProviderManagerResult & { message: string } instead of boolean, while preserving
its existing success checks so callers can safely narrow result.message to a
non-optional string.

Source: Coding guidelines


export type ConsoleOAuthFlowResult = {
type: 'oauth';
} | {
Expand Down Expand Up @@ -559,7 +574,7 @@ function OAuthStatusMessage({
<ProviderManager
mode="first-run"
onDone={result => {
if (!result || result.action !== 'saved' || !result.message) {
if (!isSuccessfulProviderSetupResult(result)) {
setOAuthStatus({ state: 'idle' })
return
}
Expand Down
Loading