refactor(router): decompose acknowledgments.ts god module into focused modules#590
Conversation
|
✨ On it — checking the router acknowledgments refactor Progress: [░░░░░░░░░░] 3% (iteration 2/70) 🔍 Code Review Update (1 min) I've started my review by examining the PR diff to understand the overall changes. I'm now focusing on key files to grasp existing patterns and ensure the proposed changes align with them. My next step will be to drill down into specific modules, starting with Last updated: iteration 2 · review |
|
✅ Review complete — APPROVED Clean god module decomposition. The extraction is behavior-preserving, backward compatibility is maintained via re-exports, the config cache design is appropriate for Node.js's single-threaded model, and all CI checks pass. No issues found. |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean god module decomposition that correctly extracts 4 concerns into focused modules while maintaining full backward compatibility through re-exports. All CI checks pass.
Notes
-
Extraction is behavior-preserving: The bot identity resolvers and GitHub token resolver are moved verbatim. The JIRA ack functions now delegate to
JiraPlatformClientwhich was already performing the same fetch logic — the only difference being the upgrade from api/2 to api/3 with ADF, which was already happening inline inacknowledgments.tsbefore this PR (the oldpostJiraAckalready usedmarkdownToAdf+ api/3). The client now owns that conversion, eliminating the duplication. -
Cache design is appropriate: The
loadProjectConfig()TTL cache uses module-level variables withDate.now()comparison. In Node.js's single-threaded model, there are no race conditions. The 5s TTL is short enough to pick up config changes without meaningful staleness. The_resetProjectConfigCache()escape hatch for tests is consistent with the existing_reset*pattern used byBotIdentityCacheand_resetJiraCloudIdCache. -
deleteCommentstill uses api/2:JiraPlatformClient.deleteComment()(line 65) uses/rest/api/2/whilepostComment()was upgraded to/rest/api/3/. This is fine — JIRA's DELETE endpoint is version-agnostic and doesn't require ADF — just noting the intentional asymmetry. -
Re-exports preserve public API: All existing import paths through
acknowledgments.tscontinue to work. The newResolvedGitHubTokennamed interface is structurally identical to the previous inline{ token: string; project: ProjectConfig }return type.
nhopeatall
left a comment
There was a problem hiding this comment.
Review already submitted — APPROVED. See #590 (review)
Summary
Refactors
src/router/acknowledgments.ts— a 242-line "God Module" mixing 4 unrelated concerns — into focused, single-responsibility modules.Changes:
src/router/bot-identity-resolvers.ts— movesresolveJiraBotAccountId(),resolveTrelloBotMemberId()and their TTL caches out of acknowledgments.ts; re-exported from acknowledgments.ts for backward compatibilitysrc/router/github-token-resolver.ts— movesresolveGitHubTokenForAck()andresolveGitHubTokenForAckByAgent()into their own module; re-exported from acknowledgments.ts for backward compatibilityJiraPlatformClient.postComment()now uses JIRA API v3 with ADF bodies (matching whatpostJiraAck()was doing inline), sopostJiraAck()anddeleteJiraAck()now delegate to the client, removing ~30 lines of duplicated fetch code and the directmarkdownToAdfimport from acknowledgments.tsloadProjectConfig()— eliminates ~10 redundant DB queries per webhook event across the adapter chain; adds_resetProjectConfigCache()for testingbot-identity-resolvers.test.tsandgithub-token-resolver.test.tsCard: https://trello.com/c/69a43240de86804a4bcf78cc
Test plan
bot-identity-resolvers.tsandgithub-token-resolver.ts(18 new tests)🤖 Generated with Claude Code