Skip to content
Merged
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
14 changes: 13 additions & 1 deletion src/api/routers/pm-discovery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}),

/**
Expand Down
79 changes: 79 additions & 0 deletions tests/unit/api/pm-discovery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
73 changes: 73 additions & 0 deletions tests/unit/web/pm-wizard-manifest-section.test.ts
Original file line number Diff line number Diff line change
@@ -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}');
});
});
20 changes: 8 additions & 12 deletions web/src/components/projects/pm-providers/manifest-section.tsx
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -20,8 +20,8 @@ export interface ManifestProviderWizardSectionProps {
readonly def: ProviderWizardDefinition;
readonly state: WizardState;
readonly dispatch: React.Dispatch<WizardAction>;
readonly projectId: string | undefined;
readonly advanceToStep: (step: number) => void;
/** Resolved once by ManifestStepsSection and shared across all step instances. */
readonly providerHooks: Record<string, unknown>;
/**
* Which step index to render. Returned as an element ready to drop into
* the caller's `<WizardStep>` wrapper. Returns null for an out-of-range
Expand All @@ -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 });
Expand Down
9 changes: 4 additions & 5 deletions web/src/components/projects/pm-providers/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading
Loading