refactor(server): extract generic webhook handler factory to eliminate triplicated code

[aaight]

Summary

• Extract a createWebhookHandler() factory in src/server/webhookHandlers.ts that replaces three near-identical 50-60 line POST handler blocks in both src/server.ts and src/router/index.ts
• Eliminate ~150 lines of duplicated capacity-checking, header extraction, JSON parsing, webhook logging, reaction dispatching, and fire-and-forget processing code
• Provide platform-specific parser helpers (parseTrelloPayload, parseGitHubPayload, parseJiraPayload) and reaction sender builders as composable, independently testable units

Test plan

• All 8 existing tests/unit/server.test.ts tests pass unchanged (verified)
• 20 new unit tests in tests/unit/server/webhookHandlers.test.ts cover:
  • Factory: 503 on capacity, 400 on parse failure, 200 on success, correct reaction/process dispatch
  • Parsers: Trello (action.type), GitHub (X-GitHub-Event header), JIRA (webhookEvent field)
  • Reaction builders: Trello (commentCard only), GitHub (issue_comment/pr_review_comment only, repo lookup), JIRA (comment_* events only)
• TypeScript: zero errors
• Biome lint: zero errors

Card: https://trello.com/c/aQ9kSuJz/83-find-top-candidate-for-refactoring-and-plan-clean-refactoring-of-it-look-for-god-classes-modules-functions-files-and-code-duplic

🤖 Generated with Claude Code

[nhopeatall]

🔍 Reviewing PR...

[nhopeatall]

Summary

Clean, well-executed refactoring that correctly eliminates ~150 lines of triplicated webhook handler code. The factory pattern is well-designed with good separation of concerns. CI is green and the new tests are thorough.

Two behavioral changes in the router deployment mode are worth noting — one is a should-fix, one is informational.

Code Issues

Should Fix

• src/router/index.ts — The old router handlers called handleTrelloWebhook/handleGitHubWebhook/handleJiraWebhook synchronously (awaited) before responding. This ensured the job was queued into the job queue before the 200 was returned to the webhook source. The new factory wraps all processing in setImmediate, making it fire-and-forget. This means the HTTP 200 is sent before the job is queued.

  In server.ts this is fine (and was the existing behavior). But in the router, this is a real regression: if the process crashes between sending the 200 and setImmediate firing, the webhook event is lost without the sender knowing. The old code guaranteed that a 200 meant "job queued." Consider either (a) making processWebhook awaited rather than fire-and-forget, or (b) adding a fireAndForget option to the factory config that the router can set to false.

• src/server/webhookHandlers.ts:113-121 — The webhook log now always records processed: true when parsing succeeds. In the old router/index.ts, the Trello handler logged processed: shouldProcess && !!project && !!cardId (reflecting whether the event was actually relevant and matched a project). Similarly, projectId was included in the old log but is now absent. This reduces observability — the dashboard webhook log viewer will show all successfully-parsed webhooks as "processed" even if they were filtered out as irrelevant events.

  This is less critical for server.ts where the old code already logged processed: true unconditionally. But for the router path, this is a loss of signal. Consider extending WebhookHandlerConfig to accept an optional resolveLogFields callback that can enrich the log after processing (or at minimum, document this as a known trade-off).

Informational

• The factory correctly defaults checkCapacity to true, and the router explicitly passes false — this is correct and well-documented.
• The reaction builders faithfully reproduce the old platform-specific logic.
• The parseGitHubPayload correctly delegates to the existing parseGitHubWebhookPayload for form-encoded support.

[nhopeatall]

The factory always uses setImmediate for fire-and-forget processing. This matches the old server.ts behavior but changes the old router/index.ts behavior, which awaited processing (job queuing) before responding.

Consider adding a fireAndForget?: boolean option (defaulting to true) so the router can opt into synchronous processing:

if (fireAndForget) {
  setImmediate(() => { processWebhook(...).catch(...); });
} else {
  await processWebhook(payload, eventType).catch(...);
}

This preserves the guarantee that the router's 200 means 'job queued.'

[aaight]

Addressed. Added a fireAndForget option to WebhookHandlerConfig (defaults to true for backward compatibility). The router now passes fireAndForget: false, restoring the original behavior where processWebhook is awaited before responding — so 200 means "job queued."

In the factory implementation:

• fireAndForget: true (server mode): logs, then schedules processWebhook via setImmediate, returns 200 immediately
• fireAndForget: false (router mode): awaits processWebhook, then logs, then returns 200

6 new unit tests cover this behavior.

[nhopeatall]

In the old router/index.ts, the Trello webhook log included projectId: project?.id and processed: shouldProcess && !!project && !!cardId, reflecting actual processing outcome. The factory now always logs processed: true on parse success, losing that signal.

This is a minor observability regression for the router deployment mode. The dashboard webhook log viewer will no longer distinguish between webhooks that were actually processed vs. filtered out as irrelevant.

[aaight]

Addressed. Added a resolveLogFields callback to WebhookHandlerConfig that allows callers to override the processed and projectId fields in the webhook log.

The router now uses this to capture the actual return values from handleTrelloWebhook / handleGitHubWebhook / handleJiraWebhook and pass them through:

• Trello: processed: shouldProcess && !!project && !!cardId, projectId: project?.id
• GitHub: processed: shouldProcess
• JIRA: processed: !!shouldProcess, projectId: project?.id

This fully restores the original observability in the webhook log viewer. When fireAndForget: false, resolveLogFields runs after processWebhook completes, so it reflects actual processing outcome.

[nhopeatall]

LGTM with minor suggestions — see inline comments about behavioral changes in router deployment mode (fire-and-forget processing timing, webhook log accuracy).

[zbigniewsobiecki]

@aaight address code review concerns

[aaight]

Request Addressed

What was asked

Address code review concerns from @nhopeatall's review.

Changes Made

1. Added fireAndForget option to WebhookHandlerConfig (defaults to true):

   • true (server mode): responds 200 immediately, processes via setImmediate
   • false (router mode): awaits processWebhook before responding, guaranteeing 200 means "job queued"
   • Router's index.ts now uses fireAndForget: false to restore original synchronous processing behavior

2. Added resolveLogFields callback to enrich webhook log entries:

   • Returns { processed, projectId } overrides based on actual processing outcome
   • Router now captures handler return values (shouldProcess, project, cardId) and maps them to log fields
   • Fully restores the observability regression in the dashboard webhook log viewer

3. 6 new unit tests covering:

   • fireAndForget: false awaits processing before responding
   • resolveLogFields enriches log in both fire-and-forget and synchronous modes
   • Default processed: true when no resolveLogFields provided
   • resolveLogFields runs after processWebhook completes (when fireAndForget: false)

4. Refactored factory internals (extracted logSuccessfulWebhook and handleProcessingError helpers) to stay under Biome's cognitive complexity threshold.

Verification

• All 33 relevant tests passing (25 webhook handler + 8 server)
• TypeScript: zero errors
• Biome lint: zero errors

[nhopeatall]

🔍 Reviewing PR...

[nhopeatall]

Summary

Good refactoring that successfully eliminates ~150 lines of triplicated webhook handling code. The factory pattern is well-designed, the type definitions are clear, and the new test suite is thorough. CI passes, existing tests unaffected. Two items worth noting:

Code Issues

Should Fix

• src/router/index.ts:49,86,118 — The lastResult closure pattern used to communicate data between processWebhook and resolveLogFields has a race condition under concurrent requests. Since lastResult is shared mutable state in the closure, two concurrent webhook requests to the same endpoint can interleave: Request A sets lastResult, then Request B sets lastResult, then Request A reads lastResult and gets B's data. This only affects webhook log accuracy (wrong processed/projectId in logs), not the HTTP response, so the blast radius is small. But it could be avoided by having resolveLogFields receive the result from processWebhook directly (e.g., the factory could pass a per-request context object, or processWebhook could return the log fields).

• src/server/webhookHandlers.ts:201 — In fireAndForget: false mode, processWebhook errors are caught and swallowed (.catch((err) => handleProcessingError(source, err))) and the handler still returns 200. This is a behavioral change from the old router code, where an exception from handleTrelloWebhook/handleGitHubWebhook/handleJiraWebhook would propagate to Hono's error handler and result in a 500 response. For webhook endpoints, returning 200 on failure may actually be preferable (avoids webhook platform retries causing duplicate processing), but this is a deliberate semantic change worth acknowledging.

[nhopeatall]

[SHOULD_FIX] The lastResult closure variable is shared across all concurrent requests to this endpoint. If two Trello webhooks arrive concurrently and their processWebhook calls interleave, one request's resolveLogFields can read the other request's result.

Since fireAndForget: false means both processWebhook and resolveLogFields are awaited within the same handler invocation, the interleaving happens when the first request suspends at its await handleTrelloWebhook(...) and the second request completes its processing before the first resumes.

The impact is limited to incorrect log entries (wrong processed flag or projectId), but it's a subtle bug that could be confusing during debugging.

Consider having the factory accept a processWebhook that returns the log overrides directly, eliminating the need for shared mutable state:

processWebhook: async (payload) => {
  const { shouldProcess, project, cardId } = await handleTrelloWebhook(payload, triggerRegistry);
  return {
    processed: shouldProcess && !!project && !!cardId,
    projectId: project?.id,
  };
},

With the factory using the return value for log enrichment instead of a separate resolveLogFields callback.

[aaight]

Addressed. Replaced the lastResult closure pattern + resolveLogFields callback with a return value from processWebhook. The factory now uses the return value directly for log enrichment — this is request-scoped and inherently safe under concurrent requests (no shared mutable state).

The router webhook handlers are also much simpler now — the IIFE closures are gone entirely:

processWebhook: async (payload) => {
  const { shouldProcess, project, cardId } = await handleTrelloWebhook(payload, triggerRegistry);
  return {
    processed: shouldProcess && !!project && !!cardId,
    projectId: project?.id,
  };
},

In fireAndForget: true mode (server), the return value is ignored since logging happens before processing starts.

[nhopeatall]

[SHOULD_FIX] In fireAndForget: false (router) mode, this .catch() swallows processWebhook errors and the handler still returns 200. The old router code let exceptions propagate to Hono's error handler (resulting in 500).

This may be intentionally safer for webhooks (avoids retry storms from the webhook platform), but it's a behavioral change worth documenting. If the old 500-on-error behavior was relied upon for monitoring or alerting, this change would silently break that.

[aaight]

Addressed. In fireAndForget: false mode, processWebhook errors now propagate directly to Hono's error handler (resulting in 500), matching the old router behavior. The .catch() wrapper has been removed from this code path:

// Before (swallowed errors):
await processWebhook(payload, eventType).catch((err) => handleProcessingError(source, err));

// After (errors propagate):
const logOverrides = await processWebhook(payload, eventType);

The .catch() is still used in fireAndForget: true mode (server) where fire-and-forget semantics require it. A new test verifies that errors propagate to Hono's error handler in synchronous mode.

[nhopeatall]

Review complete - see inline comments for two SHOULD_FIX items regarding the lastResult race condition and error swallowing behavior change.

[zbigniewsobiecki]

@aaight address code review concerns

[aaight]

Request Addressed

What was asked

Address code review concerns from @nhopeatall's second review round (two SHOULD_FIX items).

Changes Made

1. Eliminated lastResult race condition — Replaced the resolveLogFields callback + shared mutable lastResult closure with processWebhook return values for log enrichment. The factory now uses the per-request return value directly, which is inherently safe under concurrent requests (no shared state). This also significantly simplifies the router webhook handlers — the IIFE closures are gone entirely.

2. Restored error propagation in fireAndForget: false mode — Removed the .catch() wrapper from the synchronous processing path so processWebhook errors propagate to Hono's error handler (500 response). This matches the old router behavior where a processing failure was not acknowledged with 200. The .catch() remains only in fireAndForget: true mode where fire-and-forget semantics require it.

3. Updated tests — Replaced 4 resolveLogFields tests with 5 new tests covering: processWebhook return value enrichment, fire-and-forget mode ignoring return values, void return default behavior, and error propagation to Hono's error handler.

Verification

• All 34 relevant tests passing (26 webhook handler + 8 server)
• TypeScript: zero errors
• Biome lint: zero errors
• Note: 6 pre-existing failures in tests/unit/config/projects.test.ts are unrelated (GitHub token resolution tests) and exist on the base branch

[nhopeatall]

🔍 Reviewing PR...

[nhopeatall]

Summary

Clean, well-executed refactoring that extracts ~150 lines of triplicated webhook handler logic into a composable factory. The behavioral equivalence is preserved across both deployment modes (server fire-and-forget and router await-then-log), CI is green, and the new unit tests provide strong coverage of the factory's branching logic.

The factory design is sound: createWebhookHandler properly parameterizes the five axes of variation (source, parser, reaction sender, processing mode, capacity checking) while the platform-specific parsers and reaction builders are independently testable. The fireAndForget flag cleanly models the two deployment modes without conditional spaghetti.

No blocking or should-fix issues found. The code is correct, well-documented, and improves maintainability.

[nhopeatall]

LGTM - Clean refactoring with all prior review feedback addressed. Behavioral equivalence verified across both deployment modes.