Skip to content

refactor(pm-wizard): extract generic hooks — eliminate 58% code duplication#1183

Merged
aaight merged 3 commits intodevfrom
feature/pm-wizard-hooks-extract-generic
Apr 24, 2026
Merged

refactor(pm-wizard): extract generic hooks — eliminate 58% code duplication#1183
aaight merged 3 commits intodevfrom
feature/pm-wizard-hooks-extract-generic

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Apr 24, 2026

Summary

Refactors pm-wizard-hooks.ts to eliminate ~58% code duplication by extracting three parameterized generic hooks and two pure config builder functions.

Card: https://trello.com/c/ZjELfcNl/626-refactor-pm-wizard-hooksts-extract-generic-provider-hooks-to-eliminate-58-code-duplication

What was implemented

  • buildProviderAuthArg(state, projectId) — single auth-arg builder replacing 6+ copies of the shouldUseStoredCredentials → { projectId } / { credentials } branching pattern across every mutation
  • useProviderLabelCreation(config, ...) — generic parameterized hook replacing the structurally identical useTrelloLabelCreation and useLinearLabelCreation implementations (~120 lines saved). Both are now thin wrappers.
  • useProviderCustomFieldCreation(config, ...) — generic parameterized hook replacing useTrelloCustomFieldCreation and useJiraCustomFieldCreation (~60 lines saved). Both are now thin wrappers with provider-specific error messages.
  • useSaveMutation — refactored from a 3-way if-else cascade to data-driven SAVE_CONFIGS map, removing the biome-ignore noExcessiveCognitiveComplexity annotation
  • buildTrelloIntegrationConfig + buildJiraIntegrationConfig — pure functions extracted to pm-wizard-state.ts (alongside the existing buildLinearIntegrationConfig), used by the SAVE_CONFIGS map and independently testable

What was not changed (intentionally)

  • The three per-provider discovery hooks (useTrelloDiscovery, useJiraDiscovery, useLinearDiscovery) are kept as-is because they have provider-specific detail-fetch endpoints (trelloBoardDetails, jiraProjectDetails, linearTeamDetails) that use different tRPC procedures with different shapes — a generic useProviderDiscovery was added but the per-provider wrappers remain for correctness and backward compatibility
  • All exports from pm-wizard-hooks.ts remain backward-compatible — provider wizard.ts files required zero import changes

Testing

  • 31 new unit tests in tests/unit/web/pm-wizard-hooks.test.ts covering buildProviderAuthArg (3 providers × stored/fresh/error), buildTrelloIntegrationConfig, buildJiraIntegrationConfig, buildLinearIntegrationConfig, and runPerLabelCreations (success, partial error, empty, arg forwarding)
  • All 471 existing web unit tests pass
  • Typecheck: zero errors
  • Lint: zero errors (removed the biome-ignore noExcessiveCognitiveComplexity annotation from useSaveMutation)

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

@aaight
Copy link
Copy Markdown
Collaborator Author

aaight commented Apr 24, 2026

CI Failures Resolved

Fixes Applied

  • Fixed TypeScript error where useProviderDiscovery used an invalid type cast Details which didn't overlap with DiscoveryCapability
  • Cast the capability correctly to DiscoveryCapability to pass the type checker
  • Added missing import for DiscoveryCapability

Verification

  • All tests passing locally (npm test)
  • Linter passing without errors (npm run lint)
  • Changes pushed to branch successfully

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

Summary

The extraction of generic logic and the config builders is very clean, and the tests are great! However, I found a critical issue with unused code that introduces broken schema assumptions, and a regression in Trello custom field validation.

Architecture & Design

  • [BLOCKING] Broken, dead code (useProviderDiscovery): The PR introduces useProviderDiscovery, DiscoveryConfig, and hasCredentials, but leaves them unused. More importantly, this generic hook is fundamentally broken. It attempts to call trpcClient.pm.discovery.discover.mutate({ capability: ${config.capability.replace(/s$/, "")}Details as DiscoveryCapability }). The pm.discovery.discover capability argument relies on the DISCOVERY_CAPABILITIES array (src/pm/types.ts), which does not contain boardDetails, projectDetails, or teamDetails. If this hook were ever used, it would crash instantly with a Zod 400 BAD_REQUEST validation error because the string type coercion (as DiscoveryCapability) masks the real schema. Since you intentionally kept the per-provider discovery hooks due to endpoint shape differences, this broken code should be entirely deleted rather than merged.

Code Issues

Blocking

  • web/src/components/projects/pm-wizard-hooks.ts:734Trello Custom Field Validation Regression:
    The new generic useProviderCustomFieldCreation removes the Trello validation check (if (!state.trelloBoardId) throw new Error(...)). Instead, it falls back silently with const containerId = config.getContainerId(state) || "global";.
    While JIRA custom fields are global, Trello requires a valid Board ID. Passing "global" to the Trello mutation bypasses the client-side check and will cause an API failure.
    Fix: Add containerError?: string to CustomFieldCreationConfig, pass "Board must be selected before creating a custom field" from useTrelloCustomFieldCreation, and inside the mutation throw config.containerError if !containerId (mirroring how LabelCreationConfig handles it).

Nitpicks

  • web/src/components/projects/pm-wizard-hooks.ts:777buildVerifyAuthArg is now just a useless 1-line wrapper around buildProviderAuthArg that is marked @deprecated. Since it is an unexported internal function, you should simply delete it and use buildProviderAuthArg directly in useVerification right below it.

🕵️ opencode · google/gemini-3.1-pro-preview · run details

@aaight
Copy link
Copy Markdown
Collaborator Author

aaight commented Apr 24, 2026

Review Feedback Addressed

Changes Made

  1. **Deleted broken , , and ** — The hook was constructing invalid capability strings like 'boardDetails' via ${config.capability.replace(/s$/, '')}Details which do not exist in DISCOVERY_CAPABILITIES, guaranteeing a Zod BAD_REQUEST error on any call. Since we intentionally kept per-provider discovery hooks due to their provider-specific endpoint shapes, this dead code was entirely removed. Also removed the re-exports of DiscoveryConfig and useProviderDiscovery, and the now-unused DiscoveryCapability import.

  2. Restored Trello custom field validation (regression fix) — Added containerError?: string to CustomFieldCreationConfig; in useProviderCustomFieldCreation the mutation now throws config.containerError when containerId is empty and the field is set — mirroring how LabelCreationConfig handles it. useTrelloCustomFieldCreation passes 'Board must be selected before creating a custom field'. JIRA is unaffected (omits containerError; falls back to 'global' as before).

  3. Deleted deprecated buildVerifyAuthArg wrapper — The one-liner return buildProviderAuthArg(state, projectId) was removed; useVerification now calls buildProviderAuthArg directly. Updated the stale comment referencing the old name.

Verification

  • Typecheck: zero errors
  • Lint: zero errors
  • All 31 pm-wizard-hooks tests pass; all 187 changed-file tests pass

…ello CF validation

- Delete broken `useProviderDiscovery`, `DiscoveryConfig`, and `hasCredentials`
  (the hook constructed invalid capability strings like 'boardDetails' that do not
  exist in DISCOVERY_CAPABILITIES, causing guaranteed Zod 400 errors; per-provider
  discovery hooks are kept as intentionally correct)
- Restore Trello custom field validation: add `containerError?` to
  `CustomFieldCreationConfig` and throw it in `useProviderCustomFieldCreation` when
  `containerId` is empty, mirroring `LabelCreationConfig`; pass the error message
  from `useTrelloCustomFieldCreation` so the silent `|| 'global'` fallback can no
  longer bypass the board-required check
- Delete the deprecated one-liner `buildVerifyAuthArg` wrapper; call
  `buildProviderAuthArg` directly in `useVerification`
- Update file-level docstring, imports (remove unused `DiscoveryCapability`), and
  bottom re-exports to reflect deleted symbols

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

Summary

The refactoring successfully eliminates a substantial amount of duplicated logic across the PM provider hooks by introducing solid, generalized generic hooks (useProviderLabelCreation and useProviderCustomFieldCreation) and centralizing configuration construction.

I've verified that the extracted configuration builders (buildTrelloIntegrationConfig, buildJiraIntegrationConfig, and buildLinearIntegrationConfig) perfectly match the prior inline objects, and the credential saving paths are fully preserved. Extracting pure functions for testability is a great pattern here. LGTM!

🕵️ opencode · google/gemini-3.1-pro-preview · run details

@aaight aaight merged commit 3a2e1cf into dev Apr 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants