refactor(trello): extract trelloFetch helper, mapLabels utility, and credential resolver#512
Conversation
…credential resolver
|
🤔 Just a sec, looking into that Trello refactor now |
|
👍 Standing by! I'm ready to address any feedback once you've had a chance to review the refactoring changes. |
|
🔍 Reviewing code — Examining the PR changes for quality and correctness... |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean structural refactoring that reduces duplication without behavioral changes. All tests pass including 10 new ones covering the extracted helpers.
Verified:
-
trelloFetch<T>()correctly centralizes credential injection, URL construction (?vs&separator), error handling, and JSON parsing. All 11 former rawfetch()call sites now route through it with identical runtime behavior. -
mapLabels()replaces 3 identical inline mapping blocks acrossgetCard,createCard, andgetListCards. Behavior is unchanged. -
resolveFromEnvOrDb<T>()inprovider.tspreserves the 3-step env → worker → DB resolution pattern exactly. I traced all three callers (getIntegrationCredential,getIntegrationCredentialOrNull,getOrgCredential) and confirmed behavioral equivalence, including the throw-on-not-found path ingetIntegrationCredential. -
Error message format change (
"Failed to X: ${status}"→"Trello API error ${status} for ${path}") — verified no production code parses these messages. One test inreactions.test.tsreferences the old format but only as a mock input (not dependent on actual output), so it's unaffected. -
Method reordering in
trelloClientis purely cosmetic — no methods were added, removed, or had signatures changed.
Net result: 26 fewer lines, single-responsibility helpers, no risk.
Summary
trelloFetch<T>()helper insrc/trello/client.tsthat centralizes credential injection, URL construction (?vs&separator logic), error checking, and JSON parsing — eliminating 11 duplicated rawfetch()blocks (~200 lines of boilerplate)mapLabels()utility function used bygetCard,getListCards, andcreateCard— replacing 3 near-identical 5-line blocks with a single shared functiontrelloClientmethods into logical sections with clear headers (Card Ops, Comments, Labels, Attachments, Checklists, Custom Fields, Board Ops, Member/Actions)resolveFromEnvOrDb()helper insrc/config/provider.tsthat encapsulates the 3-step env/worker/DB credential resolution pattern, replacing the tripled block ingetIntegrationCredential,getIntegrationCredentialOrNull, andgetOrgCredentialtrelloFetch(URL construction,&vs?separator, error handling, request body) andmapLabels(all 3 call sites)Changes
src/trello/client.ts: 589 → 563 lines, strictly structural (zero behavioral changes)src/config/provider.ts: extractedresolveFromEnvOrDb()helper (zero behavioral changes)tests/unit/trello/client.test.ts: updated error message expectations to match newtrelloFetcherror format (Trello API error ${status}); addedtrelloFetchandmapLabelstest sectionsTest plan
npm run lintpasses (zero errors)npm run typecheckpasses (zero errors)trelloFetchandmapLabelsall passTrello card: https://trello.com/c/699c902af197bf0a5bbf9c6d
🤖 Generated with Claude Code