feat: add branch filtering for PR comments #93
Conversation
WalkthroughAdds a new public method Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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/github-app/event-processor.ts (2)
43-50: Consider adding a cache size limit to prevent unbounded growth.The in-memory cache only evicts entries via lazy TTL in
getCachedBranchor explicit deletion on PR close. If many PRs are opened butgetCachedBranchis never called (e.g., no issue_comment events), entries accumulate indefinitely.Consider adding an LRU eviction policy or periodic cleanup:
+const MAX_CACHE_SIZE = 10000; + /** Lazy TTL for cache entries (24 hours) */ const CACHE_TTL_MS = 24 * 60 * 60 * 1000;Then in
updateBranchCache:private updateBranchCache(...): void { const key = `${repo}#${prNumber}`; const existing = this.prBranchCache.get(key); if (!existing || updatedAt >= existing.updatedAt) { this.prBranchCache.set(key, { branch, updatedAt }); + // Simple eviction: if over limit, delete oldest entries + if (this.prBranchCache.size > MAX_CACHE_SIZE) { + const oldest = [...this.prBranchCache.entries()] + .sort((a, b) => a[1].updatedAt.getTime() - b[1].updatedAt.getTime()) + .slice(0, Math.floor(MAX_CACHE_SIZE * 0.1)); + oldest.forEach(([k]) => this.prBranchCache.delete(k)); + } } }Also applies to: 59-60
349-362: Consider logging when branch cannot be resolved for PR comments.If the cache misses and
installation.idis absent, branch filtering is silently skipped for PR comments. This could cause comments to be delivered to channels with branch filters that should have excluded them.if (isPrComment) { branch = this.getCachedBranch(repository.full_name, issue.number) ?? undefined; if (!branch && event.installation?.id) { branch = (await this.fetchPrBranch( repository.full_name, issue.number, event.installation.id )) ?? undefined; } + if (!branch) { + console.warn( + `Could not resolve branch for PR comment on ${repository.full_name}#${issue.number}, skipping branch filter` + ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/github-app/app.ts(1 hunks)src/github-app/event-processor.ts(8 hunks)src/index.ts(1 hunks)src/services/message-delivery-service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Store context externally - maintain stateless bot architecture with no message history, thread context, or conversation memory
Use<@{userId}>for mentions in messages AND add mentions in sendMessage options - do not use @username format
Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()
Set ID in interaction requests and match ID in responses to correlate form submissions, button clicks, and transaction/signature responses
Use readContract for reading smart contract state, writeContract for SimpleAccount operations, and execute() for external contract interactions
Fund bot.appAddress (Smart Account) for on-chain operations, not bot.botId (Gas Wallet/EOA)
Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Always check permissions using handler.hasAdminPermission() before performing admin operations like ban, redact, or pin
User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending
Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead
Use getSmartAccountFromUserId() to retrieve a user's wallet address from their userId
Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)
Files:
src/services/message-delivery-service.tssrc/github-app/app.tssrc/index.tssrc/github-app/event-processor.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Provide alt text for image attachments and use appropriate MIME types for chunked attachments (videos, screenshots)
Files:
src/services/message-delivery-service.tssrc/github-app/app.tssrc/index.tssrc/github-app/event-processor.ts
🧬 Code graph analysis (1)
src/github-app/event-processor.ts (2)
src/github-app/app.ts (1)
GitHubApp(11-82)src/formatters/webhook-events.ts (1)
formatIssueComment(162-192)
🔇 Additional comments (7)
src/services/message-delivery-service.ts (1)
183-199: LGTM! Retroactive anchor creation logic is well-structured.The fallback to
handleCreatewhen editing an anchor without an existing message enables proper handling of PRs whose base branch changes to match a filter. The early return prevents the misleading "No existing message to edit" log.src/github-app/app.ts (1)
74-81: LGTM! Clean delegation to underlying Octokit App.The method correctly exposes installation-scoped Octokit access needed for the PR branch API fallback.
src/index.ts (1)
56-60: LGTM! Dependency injection updated correctly.The
githubAppinstance is properly passed as the first argument toEventProcessor, enabling installation-scoped API access for PR branch fetching.src/github-app/event-processor.ts (4)
242-253: LGTM! Cache lifecycle properly tied to PR state.Deleting on close and updating on other actions ensures the cache stays relevant. Using
pull_request.updated_atmaintains timestamp ordering consistency.
390-396: LGTM! Cache populated from review events.Updating the cache during review events ensures branch data is available for subsequent issue_comment events on the same PR.
540-568: LGTM with minor note.The API fallback is well-implemented with proper error handling and cache population on success. The
repo.split("/")assumes validowner/repoformat which is guaranteed by GitHub webhook payloads.
507-537: LGTM! Cache utilities are correctly implemented.The
>=comparison allows idempotent updates, and lazy TTL eviction ingetCachedBranchensures stale entries don't persist across lookups.
shuhuiluo
force-pushed
the
feat/pr-comment-branch-filtering
branch
from
d9f21b3 to
14d3e3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/github-app/event-processor.ts (2)
335-385: Verify interaction between PR-comment branch resolution andBRANCH_FILTERABLE_EVENTS_SETThe PR-comment path (cache → API fallback → optional branch filter, with a graceful “skip branch filter” fallback when unresolved) is a good match for the stated objective.
One thing to double‑check: if
"comments"is ever added toBRANCH_FILTERABLE_EVENTS_SET, non‑PR issue comments will still hitprocessEventwitheventType: "comments"andbranchundefined, which would trigger the"branch-filterable but no branch provided"error. If you do plan to make"comments"officially branch‑filterable at some point, consider:
- Passing a branch for issue comments as well (e.g., default branch), or
- Special‑casing
"comments"in theisBranchFilterablecheck based onisPrComment.As implemented, this is fine as long as
"comments"stays out ofBRANCH_FILTERABLE_EVENTS_SET.
545-573: Minor hardening forfetchPrBranchinput and error handlingThe fallback fetch via installation‑scoped Octokit and subsequent cache update look correct. Two small, optional robustness tweaks you might consider:
- Guard the
repo.split("/")destructuring in caserepois ever malformed (e.g., log andreturn nullifownerorrepoNameare missing).- Optionally differentiate expected errors (e.g., 404 if the PR was deleted or the installation lost access) from unexpected ones to avoid noisy warnings in benign cases.
These aren’t blocking, but would make this path a bit more defensive in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/github-app/app.ts(1 hunks)src/github-app/event-processor.ts(8 hunks)src/index.ts(1 hunks)src/services/message-delivery-service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/services/message-delivery-service.ts
- src/index.ts
- src/github-app/app.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Store context externally - maintain stateless bot architecture with no message history, thread context, or conversation memory
Use<@{userId}>for mentions in messages AND add mentions in sendMessage options - do not use @username format
Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()
Set ID in interaction requests and match ID in responses to correlate form submissions, button clicks, and transaction/signature responses
Use readContract for reading smart contract state, writeContract for SimpleAccount operations, and execute() for external contract interactions
Fund bot.appAddress (Smart Account) for on-chain operations, not bot.botId (Gas Wallet/EOA)
Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Always check permissions using handler.hasAdminPermission() before performing admin operations like ban, redact, or pin
User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending
Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead
Use getSmartAccountFromUserId() to retrieve a user's wallet address from their userId
Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)
Files:
src/github-app/event-processor.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Provide alt text for image attachments and use appropriate MIME types for chunked attachments (videos, screenshots)
Files:
src/github-app/event-processor.ts
🧬 Code graph analysis (1)
src/github-app/event-processor.ts (2)
src/github-app/app.ts (1)
GitHubApp(11-82)src/formatters/webhook-events.ts (1)
formatIssueComment(162-192)
🔇 Additional comments (2)
src/github-app/event-processor.ts (2)
41-51: PR branch cache shape and TTL logic look soundThe in‑memory cache structure (
PrBranchCacheEntry,prBranchCache, 24h TTL) and helper methods (updateBranchCache,deleteBranchCache,getCachedBranch) look correct and align with the described behavior (last‑writer‑wins viaupdatedAtand lazy eviction). No functional issues stand out here.Also applies to: 59-61, 512-543
240-266: Cache updates on PR / review / review-comment events are consistentUsing
pull_request.base.refpluspull_request.updated_atto driveupdateBranchCache(anddeleteBranchCacheonclosed) is consistent across PR, review, and review-comment handlers and should give correct ordering even if events arrive slightly out of order. This wiring matches the intended “timestamp-ordered” cache semantics.Also applies to: 392-402, 430-440
PR comments (fired as issue_comment events) now respect branch filters. - Add in-memory PR branch cache with timestamp ordering - Cache populated from PR/review/review_comment events - API fallback on cache miss via installation Octokit - Lazy TTL (24h) + delete-on-close for garbage collection - Support retroactive anchor creation when PR base branch changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
shuhuiluo
force-pushed
the
feat/pr-comment-branch-filtering
branch
from
14d3e3a to
d6cb70d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/github-app/event-processor.ts (1)
349-367: PR comment branch resolution behavior is sensible but could be stricter if desiredThe flow for PR comments—cache lookup, then API fallback via installation Octokit, then logging and proceeding without a branch on failure—cleanly enables branch filtering when possible while degrading to pre‑existing behavior (no branch filtering) when the branch cannot be resolved.
If you want “branch filters are always respected” to be a hard guarantee rather than best effort, you might instead choose to drop delivery when
branchremains unresolved (or at least skip delivery to channels with a restrictivebranchFilter), rather than logging and sending as if unfiltered. Current behavior is fine and backwards‑compatible; this would just tighten semantics.Also applies to: 369-383
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/constants.ts(1 hunks)src/github-app/app.ts(1 hunks)src/github-app/event-processor.ts(8 hunks)src/index.ts(1 hunks)src/services/message-delivery-service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/constants.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/services/message-delivery-service.ts
- src/github-app/app.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Store context externally - maintain stateless bot architecture with no message history, thread context, or conversation memory
Use<@{userId}>for mentions in messages AND add mentions in sendMessage options - do not use @username format
Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()
Set ID in interaction requests and match ID in responses to correlate form submissions, button clicks, and transaction/signature responses
Use readContract for reading smart contract state, writeContract for SimpleAccount operations, and execute() for external contract interactions
Fund bot.appAddress (Smart Account) for on-chain operations, not bot.botId (Gas Wallet/EOA)
Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Always check permissions using handler.hasAdminPermission() before performing admin operations like ban, redact, or pin
User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending
Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead
Use getSmartAccountFromUserId() to retrieve a user's wallet address from their userId
Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)
Files:
src/github-app/event-processor.tssrc/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Provide alt text for image attachments and use appropriate MIME types for chunked attachments (videos, screenshots)
Files:
src/github-app/event-processor.tssrc/index.ts
🧬 Code graph analysis (1)
src/github-app/event-processor.ts (1)
src/github-app/app.ts (1)
GitHubApp(11-82)
🔇 Additional comments (5)
src/index.ts (1)
56-60: EventProcessor wiring withgithubAppmatches new constructorPassing
githubAppas the first argument aligns with the updatedEventProcessorsignature(githubApp, subscriptionService, messageDeliveryService); no issues spotted.src/github-app/event-processor.ts (4)
41-50: Branch cache state andGitHubAppinjection look consistentThe
PrBranchCacheEntrystructure, 24‑hourCACHE_TTL_MS, andprBranchCacheMap are straightforward, and injectingGitHubAppvia the constructor cleanly enables the API fallback without leaking that concern into callers. No correctness issues apparent.Also applies to: 59-66
243-254: PR handler keeps cache in sync with PR lifecycleUpdating the cache on all non‑
closedactions and deleting it onclosedensures you don’t retain stale branches while still having entries for active PRs. Usingpull_request.updated_atto monotically gate updates is reasonable and guards against out‑of‑order events.
395-402: Keeping cache warm on reviews and review comments is a good touchRefreshing the cache on
onPullRequestReviewandonPullRequestReviewCommentensures the base branch stays correct even if those events arrive without a preceding PR webhook in the same process lifetime. This also helps avoid unnecessary API calls later for PR comments.Also applies to: 433-440
512-573: Cache helper and API fallback implementations look robustThe
updateBranchCache/deleteBranchCache/getCachedBranchtrio correctly handles monotonic updates and TTL expiry, andfetchPrBranchuses a single repo‑scoped key plus defensive error handling and logging. The lazy TTL cleanup on access is simple and adequate for the current scale; you can always layer explicit metrics or periodic pruning later if needed.
PR comments (fired as issue_comment events) now respect branch filters.
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.