refactor: centralize formatters and reduce duplication #22
Conversation
- Create shared.ts with buildMessage utility and MessageSection interface - Move formatEvent from polling-service.ts to formatters/events-api.ts - Update webhook-events.ts to use shared buildMessage utility - Refactor formatters to use shared utility (eliminates 250+ duplicate lines) Benefits: - Single source of truth for message formatting - Better separation of concerns (service vs formatting) - Formatters organized in dedicated directory - Reduced code duplication across webhook and Events API formatters 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis change adds a new formatter module at Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/formatters/shared.ts (1)
8-51: Solid shared message builder; consider tighteningnumbercheckThis helper nicely standardizes the message structure and should make future formatter changes much easier.
One small refinement:
section.numberis currently used in a truthiness check:const repoLine = section.number ? `**${section.repository}** #${section.number}` : `**${section.repository}**`;If you ever pass
number = 0, it will be treated as “no number”. For GitHub PR/issue numbers this is fine (they start at 1), but for more generic reuse you may want to switch to an explicit undefined check:const repoLine = section.number !== undefined ? `**${section.repository}** #${section.number}` : `**${section.repository}**`;src/formatters/events-api.ts (1)
22-219: Well-structured Events API formatter; only minor optional polishThe overall
formatEventimplementation looks robust:
- Each event type validates required fields and safely bails out with
""for unsupported actions or incomplete payloads, which aligns with theif (message)guard in the polling service.- Pull request handling nicely enriches messages using
prDetailsMapand falls back to a concise representation when details aren’t available.- The webhook and Events API flows are consistent in headers, emojis, and general layout, with only expected differences (e.g., repository name vs full name).
One small optional style tweak: in the
ReleaseEventcase you currently passrepository: `${repo.name} ${release.tag_name}`,into
buildMessage, which means the tag is embedded into what conceptually is the “repository” field, and fully bolded. For clarity and future reuse, you might consider:
- Keeping
repository: repo.name, and- Moving the tag into either the
title(e.g.,"Release ${release.tag_name}") or ametadataline.That keeps
repositorysemantically consistent across all message types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/formatters/events-api.ts(1 hunks)src/formatters/shared.ts(1 hunks)src/formatters/webhook-events.ts(1 hunks)src/services/polling-service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/formatters/events-api.ts (3)
src/types/github-events-api.ts (1)
GitHubEvent(206-213)src/api/github-client.ts (1)
GitHubPullRequest(11-12)src/formatters/shared.ts (1)
buildMessage(22-52)
src/formatters/webhook-events.ts (2)
src/types/github-events-api.ts (2)
PullRequestEvent(56-59)IssuesEvent(92-95)src/formatters/shared.ts (1)
buildMessage(22-52)
🔇 Additional comments (3)
src/services/polling-service.ts (1)
8-8: Centralizing event formatting via shared formatter looks goodImporting
formatEventfrom../formatters/events-apicleanly removes service‑level formatting concerns while keeping the existingformatEvent(...)call signature andif (message)behavior intact. No issues from this wiring change.src/formatters/webhook-events.ts (2)
15-49: PR webhook formatter refactor correctly leverages shared builderThe refactor to compute
emoji/header/metadataand delegate tobuildMessagekeeps behavior clear while removing the previous string duplication. The additions/deletions summary foropenedactions is preserved viametadata, and unsupported actions cleanly return"", matching the Events API formatter’s contract.No functional issues spotted here.
55-76: Issue webhook formatter now cleanly aligned with shared layoutThe new
formatIssueimplementation mirrors the PR formatter pattern, usingbuildMessageforopenedandclosedactions and returning""otherwise. This keeps webhook issue messages structurally consistent with PR messages and the Events API formatter, with a clear mapping from payload fields to the sharedMessageSection.Looks good to me.
Moved tag to metadata array for semantic consistency. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/formatters/webhook-events.ts (4)
79-107: Inconsistent with PR refactoring objectives.This function still uses manual string construction instead of
buildMessage. While Push events have a unique structure (multiple commits), this inconsistency undermines the PR's stated goal of centralizing formatting and reducing duplication across formatters.Consider either:
- Extending
buildMessageto support multi-commit scenarios, or- Documenting why certain formatters cannot use the shared utility
127-144: Inconsistent with refactoring objectives.This function still uses manual string construction. Like
formatPush, this undermines the PR's goal of centralizing formatting logic.
146-162: Inconsistent with refactoring objectives.This function continues to use manual string construction instead of the shared
buildMessageutility.
164-184: Inconsistent with refactoring objectives.This function also bypasses the shared
buildMessageutility and uses manual construction.
🧹 Nitpick comments (1)
src/formatters/events-api.ts (1)
154-173: Consistent with webhook version but contributes to overall inconsistency.This matches the webhook
formatWorkflowRunimplementation, but both bypass the sharedbuildMessageutility. This is part of the broader pattern where only some formatters were refactored.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/formatters/events-api.ts(1 hunks)src/formatters/webhook-events.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/formatters/events-api.ts (3)
src/types/github-events-api.ts (1)
GitHubEvent(206-213)src/api/github-client.ts (1)
GitHubPullRequest(11-12)src/formatters/shared.ts (1)
buildMessage(22-52)
src/formatters/webhook-events.ts (2)
src/types/github-events-api.ts (2)
PullRequestEvent(56-59)IssuesEvent(92-95)src/formatters/shared.ts (1)
buildMessage(22-52)
🔇 Additional comments (6)
src/formatters/webhook-events.ts (3)
17-50: LGTM! Clean refactor to use shared buildMessage.The function correctly delegates to
buildMessagewhile handling PR action logic appropriately. The additions/deletions metadata is only included for opened PRs, which makes sense.
52-77: LGTM! Consistent with the refactor pattern.The function properly uses
buildMessageand handles issue actions cleanly.
109-125: LGTM! Properly refactored.The function correctly uses
buildMessagefor release events.src/formatters/events-api.ts (3)
29-75: LGTM! Robust handling of PR details.The logic correctly handles both full PR details from
prDetailsMapand provides a sensible fallback when details are unavailable. The manual URL construction on line 35 is necessary given the Events API structure.
77-104: LGTM! Clean implementation.The issue formatting correctly uses
buildMessageand handles the Events API structure appropriately.
135-152: LGTM! Consistent with the refactor pattern.Release formatting correctly uses
buildMessage.
- Add truncation indicator (...) for commit messages over 60 chars - Use comment.user.login for IssueCommentEvent author - Use prDetailsMap for PullRequestReviewEvent title/URL with fallbacks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/formatters/events-api.ts (1)
106-137: Add compare URL to push events.The webhook formatter includes a compare URL (
comparefield on line 106 of webhook-events.ts), but this Events API version omits it entirely. While the Events APIPushEventpayload doesn't include a pre-builtcomparefield, it providesbeforeandhead(or SHA) fields that can construct one:https://github.com/${repo.name}/compare/${before}...${head}. Including the compare URL makes push events more actionable.Apply this diff to add the compare URL:
case "PushEvent": { - const { commits, ref } = payload; + const { commits, ref, before, head } = payload; if (!commits || commits.length === 0) return ""; const branch = ref?.replace("refs/heads/", "") || "unknown"; const commitCount = commits.length; + const compareUrl = `https://github.com/${repo.name}/compare/${before}...${head}`; let message = `📦 **Push to ${repo.name}**\n` + `🌿 Branch: \`${branch}\`\n` + `👤 ${actor.login}\n` + `📝 ${commitCount} commit${commitCount > 1 ? "s" : ""}\n\n`; // Show first 3 commits const displayCommits = commits.slice(0, 3); for (const commit of displayCommits) { const shortSha = commit.sha.substring(0, 7); const firstLine = commit.message.split("\n")[0]; const shortMessage = firstLine.length > 60 ? firstLine.substring(0, 60) + "..." : firstLine; message += `\`${shortSha}\` ${shortMessage}\n`; } if (commitCount > 3) { message += `\n_... and ${commitCount - 3} more commit${commitCount - 3 > 1 ? "s" : ""}_`; } + message += `\n🔗 ${compareUrl}`; + return message; }Also update the
PushPayloadtype definition insrc/types/github-events-api.tsto includebefore,head,push_id,size, anddistinct_sizefields to match the actual GitHub Events API payload structure.
🧹 Nitpick comments (4)
src/formatters/events-api.ts (1)
158-177: Consider using buildMessage for consistency.The
WorkflowRunEventcase uses manual string concatenation while other event types (PullRequestEvent, IssuesEvent, ReleaseEvent) use the sharedbuildMessageutility. For consistency with the PR's goal to centralize formatting, consider refactoring this case.Apply this diff to use
buildMessage:case "WorkflowRunEvent": { const { action, workflow_run: workflowRun } = payload; if (!workflowRun) return ""; if (action === "completed") { const emoji = workflowRun.conclusion === "success" ? "✅" : "❌"; - const status = - workflowRun.conclusion === "success" ? "Passed" : "Failed"; + const header = `CI ${workflowRun.conclusion === "success" ? "Passed" : "Failed"}`; - return ( - `${emoji} **CI ${status}**\n` + - `**${repo.name}**\n` + - `🔧 ${workflowRun.name}\n` + - `🌿 ${workflowRun.head_branch}\n` + - `🔗 ${workflowRun.html_url}` - ); + return buildMessage({ + emoji, + header, + repository: repo.name, + user: actor.login, + metadata: [ + `🔧 ${workflowRun.name}`, + `🌿 ${workflowRun.head_branch}`, + ], + url: workflowRun.html_url, + }); } return ""; }src/formatters/webhook-events.ts (3)
129-146: Consider refactoring to use buildMessage for consistency.The
formatWorkflowRunfunction still uses manual string concatenation (lines 136-142) while other webhook formatters (formatPullRequest,formatIssue,formatRelease) have been refactored to usebuildMessage. For consistency with the PR's centralization goal, consider refactoring this formatter.Apply this diff to use
buildMessage:export function formatWorkflowRun(payload: WorkflowRunEvent): string { const { action, workflow_run, repository } = payload; if (action === "completed") { const emoji = workflow_run.conclusion === "success" ? "✅" : "❌"; - const status = workflow_run.conclusion === "success" ? "Passed" : "Failed"; + const header = `CI ${workflow_run.conclusion === "success" ? "Passed" : "Failed"}`; - return ( - `${emoji} **CI ${status}**\n` + - `**${repository.full_name}**\n` + - `🔧 ${workflow_run.name}\n` + - `🌿 ${workflow_run.head_branch}\n` + - `🔗 ${workflow_run.html_url}` - ); + return buildMessage({ + emoji, + header, + repository: repository.full_name, + user: workflow_run.actor.login, + metadata: [ + `🔧 ${workflow_run.name}`, + `🌿 ${workflow_run.head_branch}`, + ], + url: workflow_run.html_url, + }); } return ""; }
148-164: Consider refactoring to use buildMessage for consistency.The
formatIssueCommentfunction still uses manual string concatenation (lines 154-160) while other webhook formatters have been refactored to usebuildMessage. Although the comment quote adds custom formatting, the base structure could still benefit frombuildMessagefor consistency.Consider this refactoring approach:
export function formatIssueComment(payload: IssueCommentEvent): string { const { action, issue, comment, repository } = payload; if (action === "created") { const shortComment = comment.body.split("\n")[0].substring(0, 100); + const ellipsis = comment.body.length > 100 ? "..." : ""; - return ( - `💬 **New Comment on Issue #${issue.number}**\n` + - `**${repository.full_name}**\n\n` + - `"${shortComment}${comment.body.length > 100 ? "..." : ""}"\n` + - `👤 ${comment.user.login}\n` + - `🔗 ${comment.html_url}` - ); + return buildMessage({ + emoji: "💬", + header: `New Comment on Issue #${issue.number}`, + repository: repository.full_name, + title: `"${shortComment}${ellipsis}"`, + user: comment.user.login, + url: comment.html_url, + }); } return ""; }
166-186: Consider refactoring to use buildMessage for consistency.The
formatPullRequestReviewfunction still uses manual string concatenation (lines 176-182) while other webhook formatters have been refactored to usebuildMessage. For consistency with the PR's centralization goal, consider refactoring this formatter.Apply this diff to use
buildMessage:export function formatPullRequestReview( payload: PullRequestReviewEvent ): string { const { action, review, pull_request, repository } = payload; if (action === "submitted") { let emoji = "👀"; if (review.state === "approved") emoji = "✅"; if (review.state === "changes_requested") emoji = "🔄"; + const header = `PR Review: ${review.state.replace("_", " ")}`; - return ( - `${emoji} **PR Review: ${review.state.replace("_", " ")}**\n` + - `**${repository.full_name}** #${pull_request.number}\n\n` + - `**${pull_request.title}**\n` + - `👤 ${review.user.login}\n` + - `🔗 ${review.html_url}` - ); + return buildMessage({ + emoji, + header, + repository: repository.full_name, + number: pull_request.number, + title: pull_request.title, + user: review.user.login, + url: review.html_url, + }); } return ""; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/formatters/events-api.ts(1 hunks)src/formatters/webhook-events.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/formatters/events-api.ts (3)
src/types/github-events-api.ts (1)
GitHubEvent(206-213)src/api/github-client.ts (1)
GitHubPullRequest(11-12)src/formatters/shared.ts (1)
buildMessage(22-52)
src/formatters/webhook-events.ts (1)
src/formatters/shared.ts (1)
buildMessage(22-52)
🔇 Additional comments (8)
src/formatters/events-api.ts (4)
29-75: LGTM! Proper handling of PR details lookup.The code correctly uses
prDetailsMapto enrich minimal Events API PR objects with full details, includes a sensible fallback when details are unavailable, and uses the sharedbuildMessageutility for consistency.
77-104: LGTM! Consistent issue formatting.The issue formatter correctly uses Events API structure (
actor.login,repo.name) and delegates to the sharedbuildMessageutility.
139-156: LGTM! Proper release formatting.The release formatter correctly uses
buildMessageand provides a sensible fallback fromrelease.nametorelease.tag_name.
179-196: LGTM! Comment author correctly attributed.The code correctly uses
comment.user.login(line 191) for author attribution and includes a proper guard forcomment.user(line 182). This aligns with the webhook formatter's approach.src/formatters/webhook-events.ts (4)
17-50: LGTM! Clean refactoring using buildMessage.The PR formatter has been successfully refactored to use the shared
buildMessageutility, reducing duplication and centralizing formatting logic.
52-77: LGTM! Clean refactoring using buildMessage.The issue formatter has been successfully refactored to use the shared
buildMessageutility, consistent with the centralization goal.
79-109: LGTM! Appropriate custom formatting for push events.The push formatter uses custom string concatenation to display commit lists, which is appropriate given its unique format requirements. The updated commit message truncation (lines 96-98) correctly extracts the first line before applying the 60-character limit, preventing multiline messages from cluttering the output.
111-127: LGTM! Clean refactoring using buildMessage.The release formatter has been successfully refactored to use the shared
buildMessageutility.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/formatters/events-api.ts (1)
106-137: Compare URL still missing from PushEvent formatter.This issue was previously flagged but remains unaddressed. The Events API PushEvent payload includes
beforeandheadSHA fields that can be used to construct a compare URL (https://github.com/${repo.name}/compare/${before}...${head}), providing users with a direct link to view the changes—matching the functionality in the webhook formatter.Add the compare URL to improve user experience:
case "PushEvent": { - const { commits, ref } = payload; + const { commits, ref, before, head } = payload; if (!commits || commits.length === 0) return ""; const branch = ref?.replace("refs/heads/", "") || "unknown"; const commitCount = commits.length; + const compareUrl = before && head + ? `https://github.com/${repo.name}/compare/${before.substring(0, 7)}...${head.substring(0, 7)}` + : null; let message = `📦 **Push to ${repo.name}**\n` + `🌿 Branch: \`${branch}\`\n` + `👤 ${actor.login}\n` + `📝 ${commitCount} commit${commitCount > 1 ? "s" : ""}\n\n`; // Show first 3 commits const displayCommits = commits.slice(0, 3); for (const commit of displayCommits) { const shortSha = commit.sha.substring(0, 7); const firstLine = commit.message.split("\n")[0]; const shortMessage = firstLine.length > 60 ? firstLine.substring(0, 60) + "..." : firstLine; message += `\`${shortSha}\` ${shortMessage}\n`; } if (commitCount > 3) { message += `\n_... and ${commitCount - 3} more commit${commitCount - 3 > 1 ? "s" : ""}_`; } + if (compareUrl) { + message += `\n🔗 ${compareUrl}`; + } return message;
🧹 Nitpick comments (2)
src/formatters/events-api.ts (2)
158-177: Consider usingbuildMessagefor consistency.The WorkflowRunEvent handler manually constructs its message, while most other handlers use the shared
buildMessageutility. Refactoring this to usebuildMessagewould better align with the PR's goal of centralizing formatting and reducing duplication.case "WorkflowRunEvent": { const { action, workflow_run: workflowRun } = payload; if (!workflowRun) return ""; if (action === "completed") { const emoji = workflowRun.conclusion === "success" ? "✅" : "❌"; const status = workflowRun.conclusion === "success" ? "Passed" : "Failed"; - return ( - `${emoji} **CI ${status}**\n` + - `**${repo.name}**\n` + - `🔧 ${workflowRun.name}\n` + - `🌿 ${workflowRun.head_branch}\n` + - `🔗 ${workflowRun.html_url}` - ); + return buildMessage({ + emoji, + header: `CI ${status}`, + repository: repo.name, + title: workflowRun.name, + user: actor.login, + metadata: [`🌿 ${workflowRun.head_branch}`], + url: workflowRun.html_url, + }); } return ""; }
179-196: Consider usingbuildMessagefor consistency.The IssueCommentEvent handler manually constructs its message, while most other handlers use the shared
buildMessageutility. The comment preview could be included in themetadatafield to maintain functionality while improving consistency with the PR's centralization objectives.case "IssueCommentEvent": { const { action, issue, comment } = payload; if (!issue || !comment || !comment.user) return ""; if (action === "created") { const shortComment = comment.body.split("\n")[0].substring(0, 100); + const commentPreview = `"${shortComment}${comment.body.length > 100 ? "..." : ""}"`; - return ( - `💬 **New Comment on Issue #${issue.number}**\n` + - `**${repo.name}**\n\n` + - `"${shortComment}${comment.body.length > 100 ? "..." : ""}"\n` + - `👤 ${comment.user.login}\n` + - `🔗 ${comment.html_url}` - ); + return buildMessage({ + emoji: "💬", + header: `New Comment on Issue #${issue.number}`, + repository: repo.name, + number: issue.number, + title: issue.title, + user: comment.user.login, + metadata: [commentPreview], + url: comment.html_url, + }); } return ""; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/formatters/events-api.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/formatters/events-api.ts (3)
src/types/github-events-api.ts (1)
GitHubEvent(206-213)src/api/github-client.ts (1)
GitHubPullRequest(11-12)src/formatters/shared.ts (1)
buildMessage(22-52)
Benefits:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor