diff --git a/src/api/routers/pm-discovery.ts b/src/api/routers/pm-discovery.ts index ba69f888..db411167 100644 --- a/src/api/routers/pm-discovery.ts +++ b/src/api/routers/pm-discovery.ts @@ -194,7 +194,19 @@ export const pmDiscoveryRouter = router({ // Call through with the raw args — the adapter is responsible for // any runtime narrowing (e.g. parseContainerId). Capability + args // typing is enforced at the adapter's method signature in plans 2/3/4. - return provider.discover(input.capability, input.args as never); + try { + return await provider.discover(input.capability, input.args as never); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + if (msg.includes('HTTP error 401') || msg.includes('AUTHENTICATION_ERROR')) { + throw new TRPCError({ + code: 'UNAUTHORIZED', + message: 'Invalid or expired credentials. Please check your API key.', + cause: err, + }); + } + throw err; + } }), /** diff --git a/tests/unit/api/pm-discovery.test.ts b/tests/unit/api/pm-discovery.test.ts index 12aa6fdd..a4e08bbc 100644 --- a/tests/unit/api/pm-discovery.test.ts +++ b/tests/unit/api/pm-discovery.test.ts @@ -235,6 +235,85 @@ describe('pmDiscoveryRouter', () => { }); }); + describe('discover — 401 error mapping', () => { + // Linear returns HTTP 401 for a bad API key; the server must translate + // that to UNAUTHORIZED (not INTERNAL_SERVER_ERROR) so the wizard can + // show "invalid credentials" instead of a generic 500. + + async function registerThrowingProvider(errorMsg: string) { + _resetPMProviderRegistryForTesting(); + const { createFakePMManifest, createFakePMProvider } = await import( + '../../helpers/fakePMProvider.js' + ); + const base = createFakePMManifest(); + registerPMProvider({ + ...base, + id: 'fake-throwing', + discoveryCapabilities: { currentUser: true }, + createDiscoveryProvider: () => { + const { provider } = createFakePMProvider(); + return { + ...provider, + discover: async () => { + throw new Error(errorMsg); + }, + }; + }, + }); + } + + it('maps provider HTTP 401 error to UNAUTHORIZED tRPC code', async () => { + await registerThrowingProvider('Linear API HTTP error 401: Unauthorized'); + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await expect( + caller.discover({ + providerId: 'fake-throwing', + capability: 'currentUser', + args: {}, + }), + ).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + }); + + it('UNAUTHORIZED message mentions credentials/API key', async () => { + await registerThrowingProvider('Linear API HTTP error 401: Unauthorized'); + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await expect( + caller.discover({ + providerId: 'fake-throwing', + capability: 'currentUser', + args: {}, + }), + ).rejects.toSatisfy((err: unknown) => { + expect((err as { message: string }).message).toMatch(/credential|API key/i); + return true; + }); + }); + + it('maps AUTHENTICATION_ERROR string to UNAUTHORIZED', async () => { + await registerThrowingProvider('AUTHENTICATION_ERROR: token expired'); + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await expect( + caller.discover({ + providerId: 'fake-throwing', + capability: 'currentUser', + args: {}, + }), + ).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + }); + + it('re-throws non-auth provider errors as INTERNAL_SERVER_ERROR', async () => { + await registerThrowingProvider('Linear API HTTP error 500: Internal Server Error'); + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await expect( + caller.discover({ + providerId: 'fake-throwing', + capability: 'currentUser', + args: {}, + }), + ).rejects.toMatchObject({ code: 'INTERNAL_SERVER_ERROR' }); + }); + }); + describe('createCustomField (plan 010/1 task 2)', () => { beforeEach(async () => { _resetPMProviderRegistryForTesting(); diff --git a/tests/unit/web/pm-wizard-manifest-section.test.ts b/tests/unit/web/pm-wizard-manifest-section.test.ts new file mode 100644 index 00000000..2a68e110 --- /dev/null +++ b/tests/unit/web/pm-wizard-manifest-section.test.ts @@ -0,0 +1,73 @@ +/** + * Guards for ManifestProviderWizardSection — request-storm fix. + * + * Before the storm fix, each ManifestProviderWizardSection instance called + * `def.useProviderHooks?.()` internally. For Linear (6 steps), that + * created 6 separate React hook instances, each firing Effect 2 on mount + * → 18 batched discovery calls in production. + * + * The fix: move useProviderHooks to a single-instance ManifestStepsSection + * wrapper in pm-wizard.tsx, and pass the result down as a prop. + * + * These tests are source-level guards that will permanently prevent + * re-introducing the per-step-instance hook call. + */ + +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); +const REPO_ROOT = resolve(__dirname, '..', '..', '..'); +const MANIFEST_SECTION_PATH = resolve( + REPO_ROOT, + 'web/src/components/projects/pm-providers/manifest-section.tsx', +); +const PM_WIZARD_PATH = resolve(REPO_ROOT, 'web/src/components/projects/pm-wizard.tsx'); + +describe('ManifestProviderWizardSection — storm fix guard', () => { + it('manifest-section.tsx does NOT invoke useProviderHooks (hook must live in parent)', () => { + const source = readFileSync(MANIFEST_SECTION_PATH, 'utf8'); + // Strip comments so the assertion targets executable code only. The + // component's JSDoc explains that it receives the result via a prop; the + // actual call must not exist in code. + const codeOnly = source + .replace(/\/\*[\s\S]*?\*\//g, '') // block comments + .replace(/\/\/[^\n]*/g, ''); // line comments + expect( + codeOnly, + 'useProviderHooks must not be called inside ManifestProviderWizardSection — ' + + 'calling it N times (once per step) creates N hook instances and N discovery request sets. ' + + 'The call must live in ManifestStepsSection (pm-wizard.tsx), called exactly once.', + ).not.toContain('useProviderHooks'); + }); + + it('manifest-section.tsx accepts providerHooks as a prop', () => { + const source = readFileSync(MANIFEST_SECTION_PATH, 'utf8'); + expect( + source, + 'providerHooks must appear as a prop in ManifestProviderWizardSectionProps', + ).toContain('providerHooks'); + }); + + it('pm-wizard.tsx has a single ManifestStepsSection wrapper that calls useProviderHooks once', () => { + const source = readFileSync(PM_WIZARD_PATH, 'utf8'); + expect(source, 'ManifestStepsSection must exist in pm-wizard.tsx').toContain( + 'ManifestStepsSection', + ); + // useProviderHooks is called exactly once — inside ManifestStepsSection. + const matches = source.match(/useProviderHooks\s*\?\./g) ?? []; + expect( + matches.length, + `useProviderHooks must be called exactly once (found ${matches.length}). ` + + 'Multiple call sites recreate the storm.', + ).toBe(1); + }); + + it('pm-wizard.tsx keys ManifestStepsSection by provider id', () => { + const source = readFileSync(PM_WIZARD_PATH, 'utf8'); + expect(source).toContain('key={manifestDef.id}'); + }); +}); diff --git a/web/src/components/projects/pm-providers/manifest-section.tsx b/web/src/components/projects/pm-providers/manifest-section.tsx index 4328f497..4644469f 100644 --- a/web/src/components/projects/pm-providers/manifest-section.tsx +++ b/web/src/components/projects/pm-providers/manifest-section.tsx @@ -1,11 +1,11 @@ /** * Shell component for manifest-driven wizard rendering. * - * Rendered by `pm-wizard.tsx` only when the active provider has a - * registered `ProviderWizardDefinition`. Because the shell itself is - * conditionally rendered, `def.useProviderHooks?.(ctx)` is called - * unconditionally from inside — preserving React's rules-of-hooks - * (the shell is either mounted or not; it never toggles hooks mid-life). + * Rendered by ManifestStepsSection in `pm-wizard.tsx`, which calls + * `def.useProviderHooks?.()` exactly once and passes the result as a + * prop. This component never calls useProviderHooks directly — doing so + * would create N independent hook instances (one per step), each firing + * Effect 2 on mount and producing an N× request storm. * * Each step's React component receives `{ state, dispatch, providerHooks }` * per `ProviderWizardStepProps`. Provider-specific adapters destructure @@ -20,8 +20,8 @@ export interface ManifestProviderWizardSectionProps { readonly def: ProviderWizardDefinition; readonly state: WizardState; readonly dispatch: React.Dispatch; - readonly projectId: string | undefined; - readonly advanceToStep: (step: number) => void; + /** Resolved once by ManifestStepsSection and shared across all step instances. */ + readonly providerHooks: Record; /** * Which step index to render. Returned as an element ready to drop into * the caller's `` wrapper. Returns null for an out-of-range @@ -34,13 +34,9 @@ export function ManifestProviderWizardSection({ def, state, dispatch, - projectId, - advanceToStep, + providerHooks, stepIndex, }: ManifestProviderWizardSectionProps): ReactElement | null { - // Unconditional hook call: the shell is only mounted when `def` exists, - // so the hook is always called on every render of the shell. - const providerHooks = def.useProviderHooks?.({ state, dispatch, projectId, advanceToStep }) ?? {}; const step = def.steps[stepIndex]; if (!step) return null; return createElement(step.Component, { state, dispatch, providerHooks }); diff --git a/web/src/components/projects/pm-providers/types.ts b/web/src/components/projects/pm-providers/types.ts index b31e4c9d..045fcb4d 100644 --- a/web/src/components/projects/pm-providers/types.ts +++ b/web/src/components/projects/pm-providers/types.ts @@ -66,11 +66,10 @@ export interface ProviderWizardDefinition { /** True when all required steps report complete. */ readonly isSetupComplete: (state: WizardState) => boolean; /** - * Optional React-hook that composes provider-specific discovery / label / - * custom-field mutations. Called by the generic wizard shell component - * (`ManifestProviderWizardSection`) unconditionally from inside the shell - * itself — so the React rules-of-hooks invariant holds even though the - * shell is rendered conditionally at the pm-wizard root. + * Optional React hook that composes provider-specific discovery / label / + * custom-field mutations. The generic wizard calls this once from the + * provider-keyed manifest-step wrapper and shares the result with every + * step, so providers do not create one hook instance per step. * * The return value is passed to every step's `Component` via the * `providerHooks` prop. Each step component adapts the shape it needs. diff --git a/web/src/components/projects/pm-wizard.tsx b/web/src/components/projects/pm-wizard.tsx index 78d4bbc1..61856f14 100644 --- a/web/src/components/projects/pm-wizard.tsx +++ b/web/src/components/projects/pm-wizard.tsx @@ -11,6 +11,7 @@ import './pm-providers/jira/index.js'; import './pm-providers/linear/index.js'; import { ManifestProviderWizardSection } from './pm-providers/manifest-section.js'; import { getProviderWizard } from './pm-providers/registry.js'; +import type { ProviderWizardDefinition } from './pm-providers/types.js'; import { SaveStep } from './pm-wizard-common-steps.js'; import { useSaveMutation, useVerification } from './pm-wizard-hooks.js'; // Plan 011/5: the three legacy `pm-wizard-{trello,jira,linear}-steps.tsx` @@ -23,6 +24,8 @@ import { buildEditState, createInitialState, isStep1Complete, + type WizardAction, + type WizardState, wizardReducer, } from './pm-wizard-state.js'; import { WizardStep } from './wizard-shared.js'; @@ -50,6 +53,122 @@ function confirmProviderSwitch( ); } +// ============================================================================ +// ManifestStepsSection — single-instance hook wrapper +// ============================================================================ + +interface ManifestStepsSectionProps { + readonly manifestDef: ProviderWizardDefinition; + readonly state: WizardState; + readonly dispatch: React.Dispatch; + readonly projectId: string; + readonly advanceToStep: (step: number) => void; + readonly getStatus: ( + stepNum: number, + complete: boolean, + ) => 'pending' | 'complete' | 'error' | 'active'; + readonly openSteps: Set; + readonly toggleStep: (step: number) => void; + readonly credsReady: boolean; + readonly verifyPending: boolean; + readonly onVerify: () => void; + readonly verificationResult: { display: string } | null | undefined; + readonly verifyError: string | null | undefined; + readonly hasStoredCredentials: boolean; + readonly isEditing: boolean; +} + +/** + * Renders all manifest-driven wizard steps for a single provider. + * + * Exists so useProviderHooks is called exactly once regardless of how many + * steps the provider declares. Calling it inside ManifestProviderWizardSection + * (which is instantiated once per step) created N independent hook instances + * and an N× discovery request storm on mount. + */ +function ManifestStepsSection({ + manifestDef, + state, + dispatch, + projectId, + advanceToStep, + getStatus, + openSteps, + toggleStep, + credsReady, + verifyPending, + onVerify, + verificationResult, + verifyError, + hasStoredCredentials, + isEditing, +}: ManifestStepsSectionProps) { + // Called exactly once — the whole point of this wrapper component. + const providerHooks = + manifestDef.useProviderHooks?.({ state, dispatch, projectId, advanceToStep }) ?? {}; + + return ( + <> + {manifestDef.steps.map((step, index) => { + const stepNumber = index + 2; // step 1 is Provider picker + const isCredentials = index === 0; + return ( + toggleStep(stepNumber)} + > + + + {/* Verify Connection belongs on the first manifest step (credentials). */} + {isCredentials && ( +
+ + {!credsReady && isEditing && hasStoredCredentials && ( + Using stored credentials + )} + {verificationResult && ( +
+ + Connected as {verificationResult.display} +
+ )} + {verifyError && ( +
+ + {verifyError} +
+ )} +
+ )} + + ); + })} + + ); +} + // ============================================================================ // Main PMWizard Component // ============================================================================ @@ -137,14 +256,8 @@ export function PMWizard({ } // ---- Manifest step layout (plans 011/4 + 012/1-4) ---- - // Iterate over `manifestDef.steps`. Every PM provider owns every - // wizard step via the manifest path — credentials, container-pick, - // mappings, webhook, everything. Parent wizard only owns the provider - // picker (step 1) and the final Save step. - const renderedManifestSteps = manifestDef - ? manifestDef.steps.map((step, index) => ({ step, index })) - : []; - const saveStepNumber = renderedManifestSteps.length + 2; // +1 for provider picker, +1 for 1-indexed + // saveStepNumber: provider picker is 1, manifest steps are 2…(N+1), save is N+2. + const saveStepNumber = (manifestDef?.steps.length ?? 0) + 2; // ---- Render ---- @@ -189,73 +302,29 @@ export function PMWizard({ * provider's `wizardSpec.steps` drives its own slot count. Every * step — including webhook-url-display — renders via the manifest * path. Parent wizard owns only the provider picker (step 1) and - * the final Save step. + * the final Save step. ManifestStepsSection calls useProviderHooks + * exactly once regardless of step count (storm fix). */} - {manifestDef && - renderedManifestSteps.map((entry, idx) => { - const stepNumber = idx + 2; // 1 is Provider picker - const isCredentials = entry.step.id === manifestDef.steps[0]?.id; - return ( - toggleStep(stepNumber)} - > - - - {/* Verify Connection button belongs on the first manifest - step (credentials). Always render — edit mode with stored - credentials uses the `projectId` path on the backend, so - users can verify without re-typing the key. */} - {isCredentials && ( -
- - {!credsReady && state.isEditing && state.hasStoredCredentials && ( - Using stored credentials - )} - {state.verificationResult && ( -
- - Connected as{' '} - {state.verificationResult.display} -
- )} - {state.verifyError && ( -
- - {state.verifyError} -
- )} -
- )} - - ); - })} + {manifestDef && ( + verifyMutation.mutate()} + verificationResult={state.verificationResult} + verifyError={state.verifyError} + hasStoredCredentials={state.hasStoredCredentials} + isEditing={state.isEditing} + /> + )} {/* Save slot. */}