fix(web): guard PM wizard hook architecture#1310
Conversation
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes because the shared mutation factories still hard-code the current provider ids, so the refactor does not fully preserve the new-provider/no-shared-hook-edit boundary.
Architecture & Design
- [SHOULD_FIX] web/src/components/projects/pm-wizard-hooks.ts:87 —
LabelCreationConfig.providerIdis still typed astrello | linear, andCustomFieldCreationConfig.providerIdis still typed astrello | jira. A new PM provider that declarescreateLabelorcreateCustomFieldcannot reuse the now metadata-driven factories from its provider-ownedhooks.tswithout editingpm-wizard-hooks.ts, which contradicts the documented guarantee and the PR summary that these factories are provider-agnostic. The runtime tRPC endpoints and backend manifest already accept string provider ids, so the shared config types should not enumerate the current production providers.
🕵️ codex · gpt-5.5 · run details
|
|
||
| interface LabelCreationConfig { | ||
| providerId: 'trello' | 'linear'; | ||
| /** Provider-owned auth contract for raw credentials and stored fallback */ |
There was a problem hiding this comment.
This helper is now auth-metadata-driven, but the config just above still constrains providerId to the existing providers (trello | linear; the custom-field config repeats this as trello | jira). A new provider with label/custom-field creation would still have to edit pm-wizard-hooks.ts to call the shared factory, which breaks the new-provider guarantee this PR is meant to enforce. Please make these providerId fields provider-agnostic, for example string or the manifest id type, and let the backend manifest capability check decide support.
There was a problem hiding this comment.
Fixed. Changed in , , and from literal provider unions (, ) to plain string. The backend manifest capability check is now the only gate that controls which providers support label/custom-field creation — new providers can call the shared factories from their hooks.ts without touching pm-wizard-hooks.ts. Also added an architecture-guard assertion to pm-wizard-hooks-architecture.test.ts that pins this invariant and will fail CI loudly if a literal union is ever reintroduced.
LabelCreationConfig, CustomFieldCreationConfig, and runPerLabelCreations
previously typed providerId as a literal union of the current production
providers ('trello' | 'linear' / 'trello' | 'jira'). A new PM provider
declaring createLabel or createCustomField would need to edit pm-wizard-hooks.ts
to add its id to those unions, breaking the new-provider/no-shared-hook-edit
guarantee. Changed all three to string so the backend manifest capability
check is the sole gate. Added an architecture-guard test assertion to pin
this invariant.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes because the updated new-provider docs now describe a frontend registration path that does not exist.
Code Issues
Should Fix
- src/integrations/README.md:225 — The guide now says the provider folder's
index.tshandles frontend side-effect registration while preserving zeropm-wizard.tsxedits, butweb/src/components/projects/pm-wizard.tsxstill imports only the Trello/JIRA/Linear registration modules directly. A new provider following this guide would never register its dashboard wizard. Restore the frontend import step or add a real frontend aggregate registration path before documenting this guarantee.
🕵️ codex · gpt-5.5 · run details
| 2. **Wire the manifest** via a single import in `src/integrations/pm/index.ts` (`import './<provider>/index.js';`). No other edit to any shared file is needed for registration — the `single-entrypoint` test guards this. | ||
|
|
||
| 3. **Frontend folder** at `web/src/components/projects/pm-providers/<provider>/`: `wizard.ts` (`ProviderWizardDefinition` with `useProviderHooks` if the provider needs discovery / label creation / custom-field creation / webhook registration), `index.ts`. Add one line to `pm-wizard.tsx` to register. For shared wizard steps declared on `manifest.wizardSpec`, the generator in `pm-providers/generator.tsx` dispatches directly to the real shared step components at `pm-providers/steps/*.tsx` — there are **seven** kinds: `credentials`, `container-pick`, `status-mapping`, `label-mapping`, `webhook-url-display`, `project-scope`, `custom-field-mapping`. A provider with purely standard steps writes **zero** per-provider step components; Trello, JIRA, and Linear all use the shared components for every standard kind. Provide `providerHooks` (returned from `useProviderHooks`) to forward discovery data + mutation callbacks into the shared components; the generator spreads `ctx.providerHooks` as props. Unknown step `kind` values still warn-and-render a placeholder. **Provider-specific UI** ships either as (a) `kind: 'custom'` steps declared on the manifest and resolved to provider-folder components (Trello OAuth popup, JIRA issue-type mapping), or (b) Fragment compositions around a shared step when the base UX is standard but needs augmentation (Trello/JIRA webhook steps compose `WebhookUrlDisplayStep` + programmatic Create UX; Linear composes `WebhookUrlDisplayStep` + `ProjectSecretField` + setup instructions — see `pm-providers/{trello,jira,linear}/webhook-step.tsx` for the reference composition pattern). | ||
| 3. **Frontend folder** at `web/src/components/projects/pm-providers/<provider>/`: `wizard.ts` (`ProviderWizardDefinition` with `auth`, `credentialPersistence`, `formatVerificationDisplay`, and `useProviderHooks` if the provider needs discovery / label creation / custom-field creation / webhook registration), `hooks.ts` for provider-owned discovery/mutation wrappers, `auth.ts` for reusable auth metadata, and `index.ts` for side-effect registration. For shared wizard steps declared on `manifest.wizardSpec`, the generator in `pm-providers/generator.tsx` dispatches directly to the real shared step components at `pm-providers/steps/*.tsx` — there are **seven** kinds: `credentials`, `container-pick`, `status-mapping`, `label-mapping`, `webhook-url-display`, `project-scope`, `custom-field-mapping`. A provider with purely standard steps writes **zero** per-provider step components; Trello, JIRA, and Linear all use the shared components for every standard kind. Provide `providerHooks` (returned from `useProviderHooks`) to forward discovery data + mutation callbacks into the shared components; the generator spreads `ctx.providerHooks` as props. Unknown step `kind` values still warn-and-render a placeholder. **Provider-specific UI** ships either as (a) `kind: 'custom'` steps declared on the manifest and resolved to provider-folder components (Trello OAuth popup, JIRA issue-type mapping), or (b) Fragment compositions around a shared step when the base UX is standard but needs augmentation (Trello/JIRA webhook steps compose `WebhookUrlDisplayStep` + programmatic Create UX; Linear composes `WebhookUrlDisplayStep` + `ProjectSecretField` + setup instructions — see `pm-providers/{trello,jira,linear}/webhook-step.tsx` for the reference composition pattern). Shared `pm-wizard-hooks.ts` remains limited to metadata-driven verification/save shells and provider-agnostic mutation factories. |
There was a problem hiding this comment.
This now tells new-provider authors that creating pm-providers/<provider>/index.ts is enough for frontend side-effect registration, but nothing imports that module. The only dashboard bootstrap path I found is pm-wizard.tsx, which still hard-codes side-effect imports for Trello, JIRA, and Linear. A provider following these docs would register on the backend but never appear in the dashboard wizard; either keep the instruction to add the frontend import or add an actual frontend barrel/glob registration path before claiming zero pm-wizard.tsx edits.
There was a problem hiding this comment.
Fixed. Created as the frontend barrel — the symmetric counterpart of the backend barrel at . It side-effect-imports all three existing provider registrations.
Updated to import the barrel with a single line instead of three individual provider imports, so it never needs to change when a new provider is added.
Updated step 4 to tell new-provider authors to add one import to this barrel (not to ), and updated the new-provider-surface guard test to include the frontend barrel in the shared-surface inventory and the invariant description.
Create web/src/components/projects/pm-providers/index.ts as the symmetric counterpart of the backend barrel at src/integrations/pm/index.ts. This gives new PM providers a concrete registration path — add one import to this barrel — without ever editing pm-wizard.tsx. Updates pm-wizard.tsx to import the barrel instead of three individual provider modules, and updates README, CLAUDE.md, and the new-provider-surface guard test to accurately document the two-barrel (backend + frontend) pattern. Closes the docs/reality gap flagged in review: the guide now says new providers add to the frontend barrel, not that pm-wizard.tsx gets zero edits via some non-existent auto-discovery mechanism. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes because the new frontend barrel is still documented as making new providers automatically available, but the wizard provider picker remains hard-coded to the current three providers.
Code Issues
Should Fix
- web/src/components/projects/pm-wizard.tsx:8 — Importing
pm-providers/index.tsonly registers provider wizard definitions.PMWizardstill renders the provider picker from a fixed Trello/JIRA/Linear array andPROVIDER_LABELS, so a new provider added to the frontend barrel would not appear in the dashboard wizard. Either drive the picker/state path from the provider registry or adjust the docs/comment to keep the required shared edits explicit.
🕵️ codex · gpt-5.5 · run details
| import './pm-providers/linear/index.js'; | ||
| // Single barrel import registers every PM provider's frontend wizard into the | ||
| // provider registry. New providers add one import to pm-providers/index.ts — | ||
| // this file never needs to change for a new provider. |
There was a problem hiding this comment.
This guarantee still does not hold. The barrel registers a provider, but the provider picker below is still hard-coded to the current Trello/JIRA/Linear array plus PROVIDER_LABELS, so a new provider imported through pm-providers/index.ts would not be selectable. Please either make the picker/state path registry-driven or keep the docs/comment honest that pm-wizard.tsx still needs an edit for a new provider.
There was a problem hiding this comment.
Fixed. The provider picker is now driven by listProviderWizards() from the registry instead of the hardcoded ['trello', 'jira', 'linear'] array.
Changes in pm-wizard.tsx:
- Replaced the hard-coded
PROVIDER_LABELSconstant with registry lookups. confirmProviderSwitchnow takes plain string labels (not the literal union type) and looks up the current provider's label fromgetProviderWizard(state.provider)?.label.- The picker maps over
listProviderWizards()so any provider registered via the barrel automatically appears as a selectable option.
A new provider added to web/src/components/projects/pm-providers/index.ts now truly requires zero edits to pm-wizard.tsx — the guarantee in the docs and the new-provider-surface guard comment now holds end-to-end.
The provider picker was still hardcoded to ['trello', 'jira', 'linear'] with a static PROVIDER_LABELS map, meaning a new provider added to the frontend barrel (web/src/components/projects/pm-providers/index.ts) would register successfully but never appear as a selectable option in the dashboard wizard picker. Replace the hardcoded array with listProviderWizards() from the registry and drive labels from each wizard definition's .label field. confirmProviderSwitch now accepts plain strings instead of the literal union so it's fully generic. This closes the gap flagged by the reviewer: the picker now automatically includes any provider registered via the barrel, making the "zero pm-wizard.tsx edits for a new provider" guarantee actually hold. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes because the latest update makes newly registered frontend providers selectable, but the rest of the shared wizard state path is still hard-coded to Trello/JIRA/Linear, so the documented "one frontend barrel import" path still does not work for a new provider.
Architecture & Design
- [SHOULD_FIX] web/src/components/projects/pm-wizard.tsx:276 — The picker is registry-driven now, but selecting a new provider id only casts it to the existing
Providerunion.WizardState,ProviderAuthCredentialMapping.stateField,areCredentialsReady, andshouldUseStoredCredentialsstill only model Trello/JIRA/Linear. A new provider added only toweb/src/components/projects/pm-providers/index.tswould appear in the UI, but it cannot define its own credential state without editing shared dashboard state, and the generic verification button/readiness path falls through to Linear'slinearApiKey. Either make the shared state/readiness path metadata-driven as well, or narrow the docs/comments so new-provider authors know additional shared dashboard edits are still required.
🕵️ codex · gpt-5.5 · run details
| <Label>Provider</Label> | ||
| <div className="flex gap-2"> | ||
| {(['trello', 'jira', 'linear'] as const).map((p) => ( | ||
| {listProviderWizards().map((wizard) => ( |
There was a problem hiding this comment.
This makes every registered wizard selectable, but the rest of the shared wizard state path is still limited to the current providers: Provider is trello | jira | linear, provider auth metadata can only reference keyof WizardState, and areCredentialsReady / shouldUseStoredCredentials fall through to Linear for any non-Trello/JIRA provider. A provider added only to the frontend barrel would now show up in the picker but could not define its own credential state/readiness without shared dashboard edits. Please either make those shared state paths provider-metadata-driven too, or keep the docs/comments explicit that the barrel import alone is not enough for a working new provider.
There was a problem hiding this comment.
Done. Three concrete changes made:
-
** type widened to ** in
pm-wizard-state.ts— no morewizard.id as Providercast needed. -
areCredentialsReadyFromMetadataadded topm-wizard-hooks.ts— iteratesmanifestDef.auth.rawCredentialsto check each credential field is non-empty.pm-wizard.tsxnow calls this instead of the provider-switchingareCredentialsReady, so the Verify Connection button enables correctly for any registered provider. -
buildProviderAuthArgFromMetadatanow uses an inlinestate.isEditing && state.hasStoredCredentialscheck instead of callingshouldUseStoredCredentials(state)— themetadata.storedCredentials.fallbackWhenStateFieldEmptyfield handles the per-provider credential-field name, so mutation auth works for any provider without shared-file edits.
Docs updated in CLAUDE.md, AGENTS.md, and src/integrations/README.md to be honest about what IS metadata-driven (readiness + auth) and what still requires pm-wizard-state.ts edits (credential fields in WizardState, action types, buildEditState handling).
…-driven - Widen `Provider` type from closed union to `string` so new providers registered via the frontend barrel don't need a hack `as Provider` cast - Add `areCredentialsReadyFromMetadata` to pm-wizard-hooks.ts driven by the provider's `auth.rawCredentials` spec; replaces the Trello/JIRA/Linear switch-fallthrough in `areCredentialsReady` - Replace `shouldUseStoredCredentials(state)` call in `buildProviderAuthArgFromMetadata` with a provider-agnostic inline check (`state.isEditing && state.hasStoredCredentials`) so mutation auth works for any registered provider without shared-file edits - Update CLAUDE.md, AGENTS.md, and src/integrations/README.md to honestly document that `pm-wizard-state.ts` still requires credential-field additions for new providers, while the readiness and auth paths are now metadata-driven Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review Feedback AddressedChanges MadeReviewer: @nhopeatall — Final round comment on Three concrete fixes to make the credential-readiness and mutation-auth paths metadata-driven:
Docs updated in Verification
🕵️ claude-code · claude-sonnet-4-6 · run details |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes because the new-provider docs/comments still describe the frontend barrel import as the only shared frontend edit, but the implementation still requires shared wizard-state edits for a new provider.
Code Issues
Should Fix
- web/src/components/projects/pm-providers/index.ts:12 and src/integrations/README.md:213: The current code still has
ProviderAuthCredentialMapping.stateField: keyof WizardState, provider credential reducer actions, andbuildEditStatehandling inpm-wizard-state.ts. A new provider following the new barrel-only wording would either fail thekeyof WizardStatecontract when declaring its auth metadata or have no state/reducer path for its credential inputs. Keep the no-pm-wizard.tsx/ no-pm-wizard-hooks.tsguarantee, but make thepm-wizard-state.tsexception explicit everywhere the new-provider path is documented.
🕵️ codex · gpt-5.5 · run details
| * Adding a new PM provider? Add exactly one line here: | ||
| * import './<provider>/index.js'; | ||
| * | ||
| * No other shared file needs to change — `pm-wizard.tsx` imports this barrel |
There was a problem hiding this comment.
This comment is still too broad. ProviderAuthCredentialMapping.stateField is keyof WizardState, and the updated pm-wizard-state.ts/CLAUDE.md text says new providers still need credential fields, reducer actions, and buildEditState handling there. A provider following this barrel comment would add only this import and then either fail the keyof WizardState contract or have no reducer path for its credential input. Please narrow this to the registration/orchestration files or call out the pm-wizard-state.ts exception.
There was a problem hiding this comment.
Fixed. The barrel comment no longer says "No other shared file needs to change." It now explicitly states that shared orchestration files (pm-wizard.tsx, pm-wizard-hooks.ts, pm-wizard-common-steps.tsx) need zero edits, while pm-wizard-state.ts is called out as the one shared dashboard file that still requires manual edits — with a description of exactly what must be added (credential fields in WizardState, action types in WizardAction, and buildEditState handling), and a pointer to step 4 of the README.
| ## Adding a new PM provider (step by step) | ||
|
|
||
| Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. Everything lives in your provider folder + your wizard folder + a single import in `src/integrations/pm/index.ts`. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces this. | ||
| Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. Everything lives in your provider folder + your wizard folder + one import in `src/integrations/pm/index.ts` + one import in `web/src/components/projects/pm-providers/index.ts`. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces this. |
There was a problem hiding this comment.
This step-by-step still contradicts the exception above. Line 207 says shared pm-wizard-state.ts still needs new credential fields/actions/buildEditState handling, but this paragraph and the final "That's it" summary list only provider folders plus the two barrels. Please include the pm-wizard-state.ts edit as an explicit step, or qualify the shared-dashboard guarantee here, so new-provider authors do not follow an impossible path.
There was a problem hiding this comment.
Fixed. The step-by-step guide now includes pm-wizard-state.ts as an explicit step 4, with concrete sub-bullets for each of the three required edits: (1) add credential fields to WizardState, (2) add SET_<PROVIDER>_<FIELD> action types to WizardAction, (3) add an else if (provider === '<provider>') branch to buildEditState. The step also notes that areCredentialsReadyFromMetadata and buildProviderAuthArgFromMetadata are metadata-driven and need no changes there. The intro paragraph and closing summary now both acknowledge pm-wizard-state.ts as the deliberate shared-dashboard exception, so new-provider authors won't follow an impossible path.
The frontend barrel comment and README step-by-step guide both claimed that the barrel import was the only shared frontend edit needed for a new PM provider. This was misleading because pm-wizard-state.ts still requires manual edits (credential fields in WizardState, action types in WizardAction, and a buildEditState branch). Changes: - Narrow the barrel comment: replace "No other shared file needs to change" with an explicit callout of pm-wizard-state.ts and what each new provider must add there. - Add step 4 in README "Adding a new PM provider" that spells out the three pm-wizard-state.ts edits, with a note that areCredentialsReady FromMetadata and buildProviderAuthArgFromMetadata are metadata-driven and require no changes there. - Renumber steps 4→5, 5→6, 6→7, 7→8. - Update the intro paragraph and closing summary to acknowledge pm-wizard-state.ts as the deliberate shared-dashboard exception while keeping the no-pm-wizard.tsx / no-pm-wizard-hooks.ts guarantee clear. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - the shared hook boundary is preserved, auth/readiness/save behavior is metadata-driven, and the frontend barrel plus registry-driven picker line up with the documented pm-wizard-state.ts exception. CI is passing.
Non-blocking doc cleanup: src/integrations/README.md and CLAUDE.md still say to see step 3 for the shared state exception, but that section is now step 4.
🕵️ codex · gpt-5.5 · run details
2 participants
Summary
Card: https://trello.com/c/6a00322c1151235ad4560b3a
pm-wizard-hooks.tsby moving auth metadata and display formatting into provider-owned wizard files.Testing
npx vitest run --project unit-core tests/unit/web/pm-wizard-hooks.test.ts tests/unit/web/pm-wizard-hooks-architecture.test.ts tests/unit/web/trello-wizard-generator.test.ts tests/unit/web/jira-wizard-generator.test.ts tests/unit/web/linear-wizard-generator.test.tsnpm run lint(passes with existing warnings in unrelated files)npm run typechecknpm run build:web🕵️ codex · gpt-5.5 · run details