feat: implement granular subscription management #45
Conversation
WalkthroughThis PR adds granular GitHub subscription management: users can add or remove specific event types via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 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: 2
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
385-446: Consider normalizingnewEventTypesinside the service for robustnessThe logic to merge and deduplicate event types is sound, and validation against
ALLOWED_EVENT_TYPESis correct. However,addEventTypescurrently assumes callers will pass already-trimmed, lowercased values.To make the service API more defensive and self-contained (especially if other callers are added later), consider normalizing
newEventTypeshere:
- Trim and lowercase each incoming type.
- Optionally dedupe before validation.
For example:
- async addEventTypes( + async addEventTypes( spaceId: string, channelId: string, repoFullName: string, - newEventTypes: string[] + newEventTypes: string[] ): Promise<{ success: boolean; eventTypes?: string; error?: string }> { + const normalizedNewTypes = newEventTypes + .map(t => t.trim().toLowerCase()) + .filter(t => t.length > 0); + const subscription = await this.getSubscription( spaceId, channelId, repoFullName ); @@ - const invalidTypes = newEventTypes.filter( + const invalidTypes = normalizedNewTypes.filter( t => !allowedSet.has(t as (typeof ALLOWED_EVENT_TYPES)[number]) ); @@ - const mergedTypes = Array.from( - new Set([...existingTypes, ...newEventTypes]) + const mergedTypes = Array.from( + new Set([...existingTypes, ...normalizedNewTypes]) );This keeps handler code simpler and prevents subtle bugs if future callers pass differently cased or spaced strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands.ts(1 hunks)src/handlers/github-subscription-handler.ts(4 hunks)src/services/subscription-service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/handlers/github-subscription-handler.ts (1)
src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
src/services/subscription-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(72-109)src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
🔇 Additional comments (6)
src/commands.ts (1)
10-11: Github command description accurately reflects new granular capabilitiesThe updated description matches the new subscribe/unsubscribe behaviors and remains concise and clear. No changes needed.
src/services/subscription-service.ts (3)
3-6: Use of shared event-type constants is correctImporting
ALLOWED_EVENT_TYPESandDEFAULT_EVENT_TYPEShere keeps the service aligned with handler validation and defaults. This centralization avoids drift between layers.
350-379: getSubscription implementation is straightforward and sufficientQuery filters on
(spaceId, channelId, repoFullName)and returns the minimal fields (id,eventTypes,deliveryMode), with a clearnullfallback when no row exists. This is a good low-level helper for higher-level methods.
453-517: removeEventTypes flow and auto-deletion behavior look correctThe method correctly:
- Fetches the subscription by
(spaceId, channelId, repoFullName).- Parses stored event types.
- Computes remaining types after removal.
- Deletes the subscription entirely when no types remain, otherwise updates
eventTypesandupdatedAt.The return shape (
success,deleted,eventTypes) is clear and gives callers enough information to present accurate UI/messaging. No changes required at the service layer.src/handlers/github-subscription-handler.ts (2)
23-25: Help text accurately documents new granular event optionsThe updated usage bullets clearly describe
--eventsfor both subscribe and unsubscribe, including the full allowed list and the granular-removal behavior. This keeps the command self-discoverable.
204-210: Unsubscribe usage text and args destructuring look goodIncluding
argsin the event destructuring and updating the usage string to advertise--eventsmatches the new granular behavior and is consistent with the subscribe help. No changes needed here.
shuhuiluo
force-pushed
the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
from
5d1696d to
b422651
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
350-517: New event-type helpers look correct; consider small robustness improvementsThe
getSubscription,addEventTypes, andremoveEventTypesflows look sound: they correctly locate the row, validate additions, merge/dedup types, and delete the subscription when nothing remains.A few non-blocking refinements to consider:
When parsing
subscription.eventTypes, both methods currently keep empty tokens if the string ever contained stray commas:const existingTypes = subscription.eventTypes ? subscription.eventTypes.split(",").map(t => t.trim()) : [];You could defensively filter falsy values (
.filter(Boolean)) to avoid ever persisting blanks back to the DB.
removeEventTypesdoes not validatetypesToRemoveagainstALLOWED_EVENT_TYPES(unlikeaddEventTypes). Today the handler guarantees valid input, but if these methods are ever reused elsewhere, invalid tokens will be silently ignored. Extracting a small sharedvalidateEventTypes(string[])helper in this service and reusing it in both methods would keep API behavior consistent.Parameter ordering across service methods is a bit mixed (
getChannelSubscriptions(channelId, spaceId)vsaddEventTypes(spaceId, channelId, ...)). It’s easy to call with swapped arguments in future call sites; consider standardizing on one order or moving to an object parameter for new APIs.None of these are blockers, but tightening them would make the service more defensive and easier to use correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands.ts(1 hunks)src/handlers/github-subscription-handler.ts(4 hunks)src/services/subscription-service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/services/subscription-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(72-109)src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
src/handlers/github-subscription-handler.ts (1)
src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
🔇 Additional comments (4)
src/commands.ts (1)
9-12:githubcommand description correctly reflects new granular behaviorThe updated description succinctly matches the enhanced subscribe/unsubscribe/status capabilities; no further changes needed here.
src/services/subscription-service.ts (1)
3-6: Event-type constants import is correctly wiredUsing both
ALLOWED_EVENT_TYPESandDEFAULT_EVENT_TYPEShere is appropriate for the new event-type management methods; nothing to adjust.src/handlers/github-subscription-handler.ts (2)
19-26: Updated/githubusage text is accurate and helpfulThe top-level usage lines now clearly describe granular
--eventssupport for subscribe/unsubscribe and align with the underlying behavior.
204-336: Granular--eventsunsubscribe flow is robust and user-friendlyThe extended
handleUnsubscribelogic:
- Correctly parses
--eventsin both--events=pr,issuesand--events pr,issuesforms.- Validates requested types against
ALLOWED_EVENT_TYPESwith clear error messaging.- Computes
actuallyRemovedby intersecting the requested types withsubscription.eventTypes, so the “Removed” list now reflects what truly changed.- Falls back cleanly to full unsubscribe when
--eventsis omitted.This aligns well with the intended granular unsubscribe behavior.
Also applies to: 338-338
shuhuiluo
force-pushed
the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
from
b422651 to
c6f1ad3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/services/subscription-service.ts (1)
404-425: Normalize event types inside the service to avoid case/whitespace edge casesBoth
addEventTypesandremoveEventTypescurrently trust callers to pass canonical event type strings (trimmed, lowercase). In practice the handler does that today, but the service itself:
- Parses
subscription.eventTypeswithsplit(",").map(t => t.trim())(no.toLowerCase()).- Compares against
newEventTypes/typesToRemoveas-is, so comparisons are case-sensitive.- In
removeEventTypes,typesToRemovecoming from the handler are lowercased, butexistingTypesmay not be, which can lead to no-op removals if legacy rows or other callers ever stored mixed-case values.To make the service robust and self-contained, consider normalizing to a canonical lowercase form at the boundary of both methods, e.g.:
- When parsing
subscription.eventTypes,map(t => t.trim().toLowerCase()).- When consuming
newEventTypes/typesToRemove, also trim + lowercase before validation and merging/removal.- Persist
updatedEventTypesas a comma‑separated list of normalized, deduped tokens.Sketch:
-const existingTypes = subscription.eventTypes - ? subscription.eventTypes.split(",").map(t => t.trim()) - : []; +const existingTypes = subscription.eventTypes + ? subscription.eventTypes + .split(",") + .map(t => t.trim().toLowerCase()) + .filter(t => t.length > 0) + : []; -const mergedTypes = Array.from( - new Set([...existingTypes, ...newEventTypes]) -); +const normalizedNew = newEventTypes + .map(t => t.trim().toLowerCase()) + .filter(t => t.length > 0); +const mergedTypes = Array.from( + new Set([...existingTypes, ...normalizedNew]) +);and similarly normalize
existingTypes/typesToRemoveinremoveEventTypesbefore filtering.This keeps behavior consistent even if older data or future callers don't perfectly match the handler’s normalization.
Also applies to: 477-503
src/handlers/github-subscription-handler.ts (1)
263-347: Granular unsubscribe logic is solid; keep normalization aligned with serviceThe
--eventshandling here is thorough:
- Supports both
--events=pr,issuesand--events pr,issues.- Normalizes requested types to lowercase, filters empties, and validates against
ALLOWED_EVENT_TYPES.- Computes
actuallyRemovedas the intersection with the current subscription and reports “(none)” when over-specified.- Falls back cleanly to full unsubscribe when
--eventsis absent.Given the service-level
removeEventTypescurrently does case-sensitive comparisons onsubscription.eventTypes, it’s worth keeping the handler and service aligned on normalization. OnceSubscriptionService.removeEventTypesis updated to normalize to lowercase before filtering (as suggested in the service file), this handler can rely on that canonicalization and you avoid any divergence between what’s reported inactuallyRemovedand what the DB actually stores/removes.No changes strictly required here after that; just ensure any future tweaks to normalization happen in one place (ideally the service) and the handler continues to treat event type strings as opaque user-facing labels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands.ts(1 hunks)src/handlers/github-subscription-handler.ts(5 hunks)src/services/subscription-service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/services/subscription-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(72-109)src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
src/handlers/github-subscription-handler.ts (1)
src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
🔇 Additional comments (3)
src/services/subscription-service.ts (1)
350-379: getSubscription implementation fits schema and usageThe single-row lookup using the composite key (spaceId, channelId, repoFullName) is aligned with the unique index in the schema and provides exactly the shape needed by the handlers. No issues from a correctness or performance perspective here.
src/handlers/github-subscription-handler.ts (2)
23-25: Updated usage text clearly reflects granular event managementThe top-level usage/help now accurately documents the
--eventsflag, allowed values, and the ability to either add or remove specific event types. This will make the new behavior discoverable.
83-134: Subscribe flow correctly gates “append events” behavior on--eventsThe combination of:
hasEventsFlag = args.some(arg => arg.startsWith("--events"));- Case-insensitive lookup via
getChannelSubscriptionsto find an existing subscription.- Only calling
addEventTypeswhen bothexistingSubscriptionandhasEventsFlagare true, and otherwise returning an explicit “Already subscribed” message,gives predictable semantics and avoids silently widening a narrowly-configured subscription. The updated UX copy is also clear about how to modify existing subscriptions.
shuhuiluo
force-pushed
the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
from
c6f1ad3 to
9e25d62
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
405-410: Consider extracting the event type parsing logic.The logic to parse and normalize existing event types (split by comma, trim, lowercase, filter empty) appears identically in both
addEventTypesandremoveEventTypes. Extracting this into a private helper method would reduce duplication and ensure consistency if the normalization logic needs to change.For example:
private parseEventTypesString(eventTypes: string): string[] { return eventTypes ? eventTypes .split(",") .map(t => t.trim().toLowerCase()) .filter(t => t.length > 0) : []; }Then use it in both methods:
- const existingTypes = subscription.eventTypes - ? subscription.eventTypes - .split(",") - .map(t => t.trim().toLowerCase()) - .filter(t => t.length > 0) - : []; + const existingTypes = this.parseEventTypesString(subscription.eventTypes);Also applies to: 485-490
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands.ts(1 hunks)src/handlers/github-subscription-handler.ts(5 hunks)src/services/subscription-service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/handlers/github-subscription-handler.ts (1)
src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
src/services/subscription-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(72-109)src/constants/event-types.ts (1)
ALLOWED_EVENT_TYPES(10-22)
🔇 Additional comments (6)
src/services/subscription-service.ts (3)
350-379: LGTM! Clean retrieval method.The
getSubscriptionmethod provides a straightforward query interface with proper scoping and returns the essential fields needed for event type management.
385-453: Solid implementation of addEventTypes with proper validation.The method correctly validates new event types against
ALLOWED_EVENT_TYPES, merges with existing types using Set deduplication, and updates the database. The error messages are clear and actionable.
460-532: removeEventTypes correctly handles auto-deletion.The method properly removes specified event types and automatically deletes the subscription when no event types remain, which aligns with the PR objectives.
src/handlers/github-subscription-handler.ts (3)
83-133: Excellent handling of existing subscriptions—past review comments properly addressed.The subscribe flow now correctly:
- Only adds event types when
--eventsis explicitly provided, preventing unwanted event type widening- Performs case-insensitive matching by fetching all channel subscriptions
- Uses the canonical repo name from the database when calling
addEventTypes- Provides clear guidance when a subscription already exists without the
--eventsflagThis implementation aligns with the PR objectives and resolves all concerns from previous reviews.
263-347: Granular unsubscribe implementation is solid and accurate.The granular unsubscribe flow properly:
- Parses both
--events=valueand--events valueformats- Validates event types against
ALLOWED_EVENT_TYPESwith clear error messages- Computes the actually removed types by intersecting with the current subscription (lines 310-316), ensuring the "Removed" message is accurate even when users request removal of types that aren't subscribed
- Handles the edge case of removing all event types by automatically deleting the subscription
This addresses the past review comment about misleading "Removed" messages.
23-24: Usage messages clearly document the new granular capabilities.The updated help text accurately reflects that:
- Subscribe can add event types to existing subscriptions
- Unsubscribe supports granular removal via
--eventsAlso applies to: 220-220
shuhuiluo
force-pushed
the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
3 times, most recently
from
3706044 to
15f57f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/services/subscription-service.ts (1)
427-622: Align event-type defaulting with existing getters for robustness against null/empty rowsThe new
getSubscription,addEventTypes, andremoveEventTypesflows look correct (normalization, deduplication, and auto-delete when no types remain). One minor robustness gap is thatparseEventTypesStringreturns an empty array for falsy/emptyeventTypes, whereasgetChannelSubscriptions/getRepoSubscriberstreat falsy values asDEFAULT_EVENT_TYPES. If any legacy rows ever haveNULL/''inevent_types,removeEventTypeswould interpret that as “no events configured” and immediately delete the subscription when asked to remove anything, which is inconsistent with the display helpers.You can make behavior consistent (and future-proof) by defaulting to
DEFAULT_EVENT_TYPESinsideparseEventTypesStringwhen the stored value is null/empty:- private parseEventTypesString(eventTypes: string | null): string[] { - if (!eventTypes) return []; - return eventTypes - .split(",") - .map(t => t.trim().toLowerCase()) - .filter(t => t.length > 0); - } + private parseEventTypesString(eventTypes: string | null): string[] { + const source = + eventTypes && eventTypes.trim().length > 0 + ? eventTypes + : DEFAULT_EVENT_TYPES; + + return source + .split(",") + .map(t => t.trim().toLowerCase()) + .filter(t => t.length > 0); + }This keeps semantics aligned everywhere and guards against any unexpected/null data without changing current behavior for well-formed rows.
src/handlers/github-subscription-handler.ts (1)
244-250: Granular unsubscribe flow is solid; consider clearer messaging when nothing changedThe
--eventsparsing/validation and delegation toremoveEventTypesare correct, and computingactuallyRemovedfixes the earlier mismatch between requested vs. effective removals. One small UX nit: whenactuallyRemovedis empty (none of the requested types were enabled), the message still says “✅ Updated subscription” even though nothing actually changed.You could keep the current behavior but slightly tweak the header when
actuallyRemoved.length === 0to make that explicit:- } else { - const removedLabel = - actuallyRemoved.length > 0 ? actuallyRemoved.join(", ") : "(none)"; - await handler.sendMessage( - channelId, - `✅ **Updated subscription to ${repo}**\n\n` + - `Removed: **${removedLabel}**\n` + - `Remaining: **${formatEventTypes(removeResult.eventTypes!)}**` - ); - } + } else { + const removedLabel = + actuallyRemoved.length > 0 ? actuallyRemoved.join(", ") : "(none)"; + const header = + actuallyRemoved.length > 0 + ? `✅ **Updated subscription to ${repo}**\n\n` + : `ℹ️ **Subscription to ${repo} unchanged**\n\n`; + + await handler.sendMessage( + channelId, + header + + `Removed: **${removedLabel}**\n` + + `Remaining: **${formatEventTypes(removeResult.eventTypes!)}**` + ); + }This keeps the logic as-is while making the “no-op” case clearer to users.
Also applies to: 292-377
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands.ts(1 hunks)src/handlers/github-subscription-handler.ts(5 hunks)src/services/subscription-service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/services/subscription-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(72-109)src/constants.ts (1)
ALLOWED_EVENT_TYPES(10-22)
src/handlers/github-subscription-handler.ts (1)
src/constants.ts (1)
ALLOWED_EVENT_TYPES(10-22)
🔇 Additional comments (3)
src/services/subscription-service.ts (1)
10-14: ALLOWED_EVENT_TYPES import keeps validation co-located with subscription persistenceImporting
ALLOWED_EVENT_TYPEShere and validating in the service layer (in addition to the handler) is a good separation-of-concerns choice and ensures DB state can’t drift to unsupported event types even if new callers bypass the handler.src/handlers/github-subscription-handler.ts (2)
25-27: Updated usage string clearly advertises granular event managementThe revised usage text for
/githubnow clearly explains both adding event types on subscribe and removing specific ones on unsubscribe, and the event-type list matches the allowed set plusallfor the subscribe path.
92-143: Existing-subscription subscribe path behavior now matches intended semanticsThe combination of
hasEventsFlagand a case-insensitive lookup viagetChannelSubscriptionscleanly differentiates between:
- “append events” when
--eventsis present, and- “already subscribed” when it’s not,
while reusing the stored repo casing for subsequent operations. This avoids silent widening of event sets and keeps UX predictable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/services/subscription-service.ts (2)
462-525: Consider early return for empty event types.The method correctly validates and merges event types. However, if
newEventTypescontains only whitespace or is empty,normalizedNew(line 485-487) will be empty, and the code will still proceed to update the database with an unchangedeventTypesfield and a newupdatedAttimestamp.Adding an early check would avoid unnecessary DB writes:
const normalizedNew = newEventTypes .map(t => t.trim().toLowerCase()) .filter(t => t.length > 0); +if (normalizedNew.length === 0) { + return { + success: true, + eventTypes: subscription.eventTypes, + }; +} + const allowedSet = new Set(ALLOWED_EVENT_TYPES);
532-599: Consider early return for empty removal list.The method correctly removes event types and deletes the subscription when no types remain. However, if
typesToRemovecontains only whitespace or is empty,normalizedRemove(lines 560-562) will be empty, andremainingTypeswill equalexistingTypes, resulting in an unnecessary DB update with an unchangedeventTypesfield.Adding an early check would avoid the unnecessary write:
const normalizedRemove = typesToRemove .map(t => t.trim().toLowerCase()) .filter(t => t.length > 0); +if (normalizedRemove.length === 0) { + return { + success: true, + deleted: false, + eventTypes: subscription.eventTypes, + }; +} + // Remove specified types const remainingTypes = existingTypes.filter( t => !normalizedRemove.includes(t) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/github-subscription-handler.ts(5 hunks)src/services/subscription-service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/services/subscription-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(72-109)src/constants.ts (2)
ALLOWED_EVENT_TYPES(10-22)DEFAULT_EVENT_TYPES(5-5)
src/handlers/github-subscription-handler.ts (1)
src/constants.ts (1)
ALLOWED_EVENT_TYPES(10-22)
🔇 Additional comments (8)
src/services/subscription-service.ts (3)
10-10: LGTM!The import of
ALLOWED_EVENT_TYPESis necessary for validating event types in the newaddEventTypesmethod.
427-456: LGTM!The
getSubscriptionmethod correctly queries for a specific subscription using the unique combination ofspaceId,channelId, andrepoFullName, and efficiently returns the first match or null.
611-627: LGTM!The
parseEventTypesStringhelper correctly normalizes event type strings with proper defaulting toDEFAULT_EVENT_TYPESand consistent lowercase normalization.src/handlers/github-subscription-handler.ts (5)
25-26: LGTM!The updated usage text clearly documents the new granular event-type management capabilities, including both the ability to add event types to existing subscriptions and remove specific event types.
92-92: LGTM!The
hasEventsFlagdetection correctly identifies when the user has explicitly provided an--eventsflag, enabling the logic to distinguish between adding event types to an existing subscription versus preventing unintended modifications.
105-142: LGTM! Previous review concerns properly addressed.The existing subscription handling correctly:
- Uses case-insensitive matching via
getChannelSubscriptionsto find subscriptions regardless of user input casing- Only adds event types when
--eventsflag is explicitly provided, preventing unexpected widening of the event set- Uses the canonical repository name from the database for the
addEventTypescall- Provides clear guidance when users attempt to subscribe to an already-subscribed repository without the
--eventsflagThese changes properly address the previous review comments about case-sensitivity and unexpected behavior.
244-249: LGTM!The updated unsubscribe usage text and argument extraction correctly support the new granular event-type removal feature.
292-381: LGTM! Previous review concern about misleading "Removed" message properly addressed.The granular unsubscribe logic correctly:
- Parses the
--eventsflag in both--events=valueand--events valueformats- Validates event types against
ALLOWED_EVENT_TYPESwith clear error messages- Computes
actuallyRemovedby intersecting requested types with the subscription's current event types, ensuring the "Removed" list accurately reflects what was actually removed- Provides context-appropriate messages: "Updated subscription" when types were removed, "unchanged" when no matching types were found, or "Unsubscribed" when all types were removed
- Falls back to full unsubscribe when no
--eventsflag is presentThis implementation properly addresses the previous review comment about displaying accurate removal information.
shuhuiluo
force-pushed
the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
from
d6a3e41 to
b998f75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/services/subscription-service.ts (2)
772-842: Consider verifying that the database update succeeded.The method checks that the subscription exists (line 778-782), then updates it (line 824-836). However, it doesn't verify that the update affected any rows. If the subscription is deleted between the check and the update (unlikely but possible), the method will return success even though nothing changed.
Consider using
.returning()to verify the update succeeded:- await db + const result = await db .update(githubSubscriptions) .set({ eventTypes: updatedEventTypes, updatedAt: new Date(), }) .where( and( eq(githubSubscriptions.spaceId, spaceId), eq(githubSubscriptions.channelId, channelId), eq(githubSubscriptions.repoFullName, repoFullName) ) - ); + ) + .returning({ id: githubSubscriptions.id }); + + if (result.length === 0) { + return { + success: false, + error: `Subscription no longer exists for ${repoFullName}`, + }; + }
849-924: Consider verifying database operations succeeded.Similar to
addEventTypes, this method has two potential robustness improvements:
- The update operation (lines 905-917) doesn't verify rows were affected
- The
unsubscribecall (line 896) returns a boolean but the result isn't checkedConsider:
if (remainingTypes.length === 0) { - await this.unsubscribe(channelId, spaceId, repoFullName); + const deleted = await this.unsubscribe(channelId, spaceId, repoFullName); + if (!deleted) { + return { + success: false, + error: `Failed to delete subscription for ${repoFullName}`, + }; + } return { success: true, deleted: true, }; } // Update subscription with remaining types const updatedEventTypes = remainingTypes.join(","); - await db + const result = await db .update(githubSubscriptions) .set({ eventTypes: updatedEventTypes, updatedAt: new Date(), }) .where( and( eq(githubSubscriptions.spaceId, spaceId), eq(githubSubscriptions.channelId, channelId), eq(githubSubscriptions.repoFullName, repoFullName) ) - ); + ) + .returning({ id: githubSubscriptions.id }); + + if (result.length === 0) { + return { + success: false, + error: `Subscription no longer exists for ${repoFullName}`, + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/commands.ts(1 hunks)src/handlers/github-subscription-handler.ts(5 hunks)src/services/subscription-service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/services/subscription-service.ts (3)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(71-108)src/constants.ts (2)
ALLOWED_EVENT_TYPES(10-22)DEFAULT_EVENT_TYPES(5-5)
src/handlers/github-subscription-handler.ts (1)
src/constants.ts (1)
ALLOWED_EVENT_TYPES(10-22)
🔇 Additional comments (6)
src/services/subscription-service.ts (2)
737-766: LGTM!The
getSubscriptionmethod is straightforward and correctly queries the subscription using the compound key (spaceId, channelId, repoFullName). The use oflimit(1)is appropriate.
938-954: LGTM!The
parseEventTypesStringhelper correctly normalizes event type strings to lowercase tokens and provides a sensible default. The split-trim-lowercase-filter pattern is consistent with the handler's event type parsing.src/handlers/github-subscription-handler.ts (4)
25-27: LGTM!The updated usage messages accurately reflect the new granular event management capabilities for both subscribe and unsubscribe commands.
92-142: Excellent implementation of granular subscribe!The enhanced subscribe flow correctly addresses the previous review feedback:
- Case-insensitive matching prevents duplicate subscriptions with different casing
- The
hasEventsFlaggate prevents unexpected widening of event types when--eventsis not specified- Clear messaging guides users when a subscription already exists
The logic is sound and the user experience is intuitive.
244-249: LGTM!The updated unsubscribe signature and usage message correctly reflect the new optional
--eventsparameter for granular event removal.
292-381: Excellent granular unsubscribe implementation!This implementation correctly addresses the previous review feedback by computing the actually-removed types (lines 340-345) and using that for accurate messaging:
- Shows "Removed: (none)" when requested types weren't enabled
- Displays appropriate header: "✅ Updated" vs "ℹ️ unchanged" based on actual changes
- Handles full deletion when all types are removed
- Falls back to complete unsubscribe when
--eventsis omittedThe validation against
ALLOWED_EVENT_TYPESand support for both--events=valueand--events valueformats are well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
772-850: Consider using transactions for atomic read-modify-update operations.Both
addEventTypesandremoveEventTypesfollow a read-then-update pattern that could lead to lost updates if concurrent modifications occur. While unlikely in typical chat bot usage, wrapping the read and update in a transaction would guarantee consistency.Example pattern:
await db.transaction(async tx => { const subscription = await tx .select({...}) .from(githubSubscriptions) .where(...) .limit(1) .for('update'); // Lock the row // Perform modifications... await tx .update(githubSubscriptions) .set({...}) .where(...); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/subscription-service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/subscription-service.ts (1)
src/constants.ts (2)
ALLOWED_EVENT_TYPES(10-22)DEFAULT_EVENT_TYPES(5-5)
🔇 Additional comments (3)
src/services/subscription-service.ts (3)
857-946: Implementation correctly handles automatic deletion when all event types are removed.The logic properly implements the PR objective: when no event types remain after removal (line 903), it calls
unsubscribeto delete the subscription entirely. The discriminated union return type with thedeletedflag clearly communicates this behavior to callers.
960-976: Well-designed helper promotes consistency and code reuse.The
parseEventTypesStringhelper properly normalizes event types to lowercase, handles null/empty inputs by defaulting toDEFAULT_EVENT_TYPES, and filters out empty strings. This ensures consistent event type handling acrossaddEventTypesandremoveEventTypes.
737-766: ****The original concern about
eventTypesnullability is unfounded. The database schema declareseventTypeswith.notNull().default(DEFAULT_EVENT_TYPES), confirming the column cannot be null. The return type declaration ofeventTypes: stringcorrectly matches the schema definition. TheparseEventTypesStringhelper acceptingstring | nullis defensive programming and does not indicate a mismatch with the actual return type.Likely an incorrect or invalid review comment.
shuhuiluo
force-pushed
the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
2 times, most recently
from
ff4571b to
b07c314
Compare
Add ability to add/remove specific event types from subscriptions instead of only full subscribe/unsubscribe operations. New commands: - `/github subscribe owner/repo --events pr,issues` - subscribe to specific types - `/github subscribe owner/repo --events releases` - add types to existing sub - `/github unsubscribe owner/repo --events pr` - remove specific types Key changes: - Add addEventTypes/removeEventTypes methods to SubscriptionService - Validate repo access via OAuth token for all subscription modifications - Split handlers into focused functions (handleNewSubscription, handleUpdateSubscription, handleRemoveEventTypes, handleFullUnsubscribe) - Extract handleInvalidOAuthToken helper to deduplicate OAuth validation - Add unsubscribe-update redirect action for OAuth callback flow - Show remaining event types after partial unsubscribe 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
shuhuiluo
force-pushed
the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
from
b07c314 to
5741793
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/handlers/github-subscription-handler.test.ts (1)
110-114: InconsistenteventTypesmock data type.This mock uses
eventTypes: DEFAULT_EVENT_TYPES(a string), while Line 132 and other tests useDEFAULT_EVENT_TYPES.split(",") as EventType[](an array). The handler/service likely expects an array, which could cause test failures or mask real bugs.mockSubscriptionService.getChannelSubscriptions = mock(() => Promise.resolve([ - { repo: "owner/repo", eventTypes: DEFAULT_EVENT_TYPES }, + { repo: "owner/repo", eventTypes: DEFAULT_EVENT_TYPES.split(",") as EventType[] }, ]) );
🧹 Nitpick comments (7)
src/utils/oauth-helpers.ts (1)
115-173: Clean implementation of centralized OAuth error handling.The switch correctly handles all current non-Valid token statuses with appropriate user messaging. The Unknown case appropriately uses a non-editable message since it represents a transient state.
Consider adding a default exhaustiveness check for future-proofing if
TokenStatusever gains new values:case TokenStatus.Unknown: { // ... existing code ... return; } + + default: { + const _exhaustive: never = tokenStatus; + console.error("Unhandled token status:", _exhaustive); + return; + } } }src/routes/oauth-callback.ts (3)
109-137: Verifysubscribe-updatehandler covers error propagation fromaddEventTypes.The handler correctly processes the update flow. However,
addEventTypesmay throw an Error if the OAuth token is missing (line 824-827 in subscription-service.ts). This thrown error would propagate to the top-level catch block (line 199), which returns a generic "Authorization failed" message - potentially confusing for the user since OAuth was already validated at the start of the callback.Consider wrapping the
addEventTypescall in a try-catch to provide a more specific error message:- const updateResult = await subscriptionService.addEventTypes( - townsUserId, - spaceId, - channelId, - redirectData.repo, - eventTypes - ); + let updateResult; + try { + updateResult = await subscriptionService.addEventTypes( + townsUserId, + spaceId, + channelId, + redirectData.repo, + eventTypes + ); + } catch (error) { + console.error("Failed to add event types:", error); + await bot.sendMessage(channelId, `❌ Failed to update subscription. Please try again.`); + return renderSuccess(c); + }
127-128: Non-null assertion oneventTypesis safe but fragile.The
updateResult.eventTypes!assertion is safe here because theaddEventTypesmethod always returnseventTypeswhensuccess: true. However, this creates a coupling with the implementation details. Consider adding a defensive check or updating the return type to makeeventTypesnon-optional on success.
139-174: Apply same error handling consideration tounsubscribe-updatehandler.Similar to
subscribe-update, theremoveEventTypescall can throw if the OAuth token is unexpectedly missing. Consider wrapping in try-catch for consistent, user-friendly error handling.src/services/subscription-service.ts (3)
115-116: Consider adding defensive validation for repo format.The comment states "caller validates format" but if an invalid format is passed (e.g., missing
/), the destructuring on line 116 will silently assignundefinedtorepo, which could lead to confusing downstream errors.- // Parse owner/repo (caller validates format) - const [owner, repo] = repoIdentifier.split("/"); + // Parse owner/repo (caller validates format) + const [owner, repo] = repoIdentifier.split("/"); + if (!owner || !repo) { + return { + success: false, + requiresInstallation: false, + error: "Invalid repository format. Use owner/repo format.", + }; + }
814-893: Extract shared validation logic to reduce duplication.
addEventTypesandremoveEventTypeshave nearly identical OAuth token retrieval and repository validation logic (lines 821-838 and 913-930). Consider extracting to a private helper:private async validateUserRepoAccess( townsUserId: string, repoFullName: string ): Promise<{ success: true } | { success: false; error: string }> { const githubToken = await this.oauthService.getToken(townsUserId); if (!githubToken) { throw new Error("OAuth token not found..."); } const [owner, repo] = repoFullName.split("/"); try { await validateRepository(githubToken, owner, repo); return { success: true }; } catch { return { success: false, error: "You don't have access to this repository" }; } }Also applies to: 901-1002
862-864: Based on my investigation, I've verified the concerns raised in the original review comment. Here are my findings:Key Findings:
Data Flow Analysis: The
subscription.eventTypescomes fromparseEventTypes()(line 1006-1009), which performs an unchecked type assertion:(eventTypes.split(",") as EventType[])with no validation.Handler Validation: The
newEventTypesparameter is validated by callers in the handler (usingALLOWED_EVENT_TYPES_SET.has()), so it should be safe.Type Assertion Gap: The cast
as EventType[]at line 862-864 indeed masks a validation gap. If the database ever contains corrupted or invalid event types, they would silently pass through.Practical Risk: In normal operation, this risk is low because:
- Handlers validate before inserting into the database
- No other code paths write directly to the
eventTypesfield- Database should only contain valid types
Missing Validation: The
ALLOWED_EVENT_TYPES_SETexists but is only used in the handler'sparseEventTypes, not in the service'sparseEventTypes.
Type assertion may mask invalid event types from DB.
The cast
as EventType[]assumes merged values are valid EventTypes. WhilenewEventTypesis validated by callers,subscription.eventTypescomes fromparseEventTypes()at line 1006, which performs an unchecked cast without validating againstALLOWED_EVENT_TYPES_SET. If the database is ever corrupted or tampered with, invalid event types would silently bypass the type system. TheSetdeduplication correctly handles duplicates, but runtime validation is missing from the retrieval path.Consider adding validation in
parseEventTypes(): useALLOWED_EVENT_TYPES_SETto filter invalid values or throw an error on corruption detection.function parseEventTypes(eventTypes: string | null): EventType[] { const parsed = eventTypes?.split(",") ?? []; return parsed.filter(e => ALLOWED_EVENT_TYPES_SET.has(e as EventType)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/formatters/subscription-messages.ts(1 hunks)src/handlers/github-subscription-handler.ts(10 hunks)src/routes/oauth-callback.ts(3 hunks)src/services/subscription-service.ts(9 hunks)src/types/oauth.ts(1 hunks)src/utils/oauth-helpers.ts(2 hunks)tests/unit/handlers/github-subscription-handler.test.ts(11 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/handlers/github-subscription-handler.tstests/unit/handlers/github-subscription-handler.test.tssrc/utils/oauth-helpers.tssrc/types/oauth.tssrc/formatters/subscription-messages.tssrc/services/subscription-service.tssrc/routes/oauth-callback.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/handlers/github-subscription-handler.tstests/unit/handlers/github-subscription-handler.test.tssrc/utils/oauth-helpers.tssrc/types/oauth.tssrc/formatters/subscription-messages.tssrc/services/subscription-service.tssrc/routes/oauth-callback.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T03:24:12.463Z
Learnt from: CR
Repo: HereNotThere/bot-github PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:24:12.463Z
Learning: Applies to **/*.ts : Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Applied to files:
src/handlers/github-subscription-handler.tssrc/utils/oauth-helpers.tssrc/routes/oauth-callback.ts
📚 Learning: 2025-11-25T03:24:12.463Z
Learnt from: CR
Repo: HereNotThere/bot-github PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:24:12.463Z
Learning: Applies to **/*.ts : Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Applied to files:
src/utils/oauth-helpers.tssrc/services/subscription-service.ts
🧬 Code graph analysis (5)
src/handlers/github-subscription-handler.ts (4)
src/constants.ts (2)
ALLOWED_EVENT_TYPES(17-29)EventType(34-34)src/types/bot.ts (1)
SlashCommandEvent(13-15)src/utils/oauth-helpers.ts (1)
handleInvalidOAuthToken(115-173)src/utils/stripper.ts (1)
stripMarkdown(6-13)
src/utils/oauth-helpers.ts (3)
src/services/github-oauth-service.ts (1)
GitHubOAuthService(65-540)src/types/oauth.ts (1)
RedirectAction(12-12)src/constants.ts (1)
EventType(34-34)
src/formatters/subscription-messages.ts (1)
src/constants.ts (1)
EventType(34-34)
src/services/subscription-service.ts (4)
src/constants.ts (2)
EventType(34-34)DEFAULT_EVENT_TYPES_ARRAY(2-7)src/formatters/subscription-messages.ts (2)
formatDeliveryInfo(6-13)formatSubscriptionSuccess(18-27)src/db/schema.ts (1)
githubSubscriptions(71-108)src/api/user-oauth-client.ts (1)
validateRepository(32-76)
src/routes/oauth-callback.ts (2)
src/constants.ts (2)
EventType(34-34)DEFAULT_EVENT_TYPES_ARRAY(2-7)src/views/oauth-pages.ts (1)
renderSuccess(32-59)
🔇 Additional comments (18)
src/types/oauth.ts (1)
6-11: LGTM!The extended
RedirectActionSchemaenum cleanly supports the new granular subscription update flows. The naming convention is consistent with existing actions.src/utils/oauth-helpers.ts (1)
3-7: LGTM!The new imports are correctly scoped to support the centralized OAuth error handling function.
src/formatters/subscription-messages.ts (1)
18-27: LGTM!The success message formatter cleanly composes the subscription confirmation with proper GitHub link formatting.
tests/unit/handlers/github-subscription-handler.test.ts (3)
48-53: LGTM!The mock service correctly includes the new
removeEventTypes,registerPendingMessage, andsendSubscriptionSuccessmethods aligned with the updated service API.
345-371: Well-structured polling mode test.The test properly verifies that
sendSubscriptionSuccessis called with the polling result and correct arguments. The use ofas constfor literal types improves type safety.
479-506: LGTM!The unsubscribe test correctly verifies the
removeEventTypescall with the expected arguments includinguserId,spaceId,channelId,repo, andeventTypesarray.src/handlers/github-subscription-handler.ts (7)
16-17: LGTM!The import of the centralized
handleInvalidOAuthTokenhelper consolidates OAuth error handling across flows.
76-150: Clean refactoring of subscribe flow.The separation of concerns with
hasEventsFlaggating the update path addresses the prior review concern about unexpectedly widening event sets. The case-insensitive matching viatoLowerCase()ensures consistent behavior across subscribe/unsubscribe paths.
155-213: LGTM!The
handleNewSubscriptionfunction cleanly handles the OAuth validation, subscription creation, and error cases. Delegating success messaging tosendSubscriptionSuccesscentralizes the formatting logic.
218-266: LGTM!The
handleUpdateSubscriptionfunction correctly uses thesubscribe-updateredirect action for OAuth flows and properly validates access before modifying the subscription.
352-454: Well-implemented granular event removal.The
actuallyRemovedcomputation (Lines 416-418) correctly addresses the prior review concern about misleading "Removed" messages. The differentiated header (✅ Updatedvsℹ️ unchanged) provides clear feedback to users.
459-501: Clarify redirect action for full unsubscribe.The function uses
"unsubscribe-update"as the redirect action (Line 480), but this is a full unsubscribe operation, not a partial update. While functionally this works (the OAuth callback likely handles both cases identically), the action name could be semantically misleading.If the OAuth callback treats
unsubscribe-updatethe same regardless of whether it's partial or full removal, this is fine. Otherwise, consider whether a dedicated"unsubscribe"action should be added toRedirectActionSchemafor clarity.
539-549: LGTM!The
parseRepoArghelper cleanly extracts markdown stripping and format validation into a reusable function.src/routes/oauth-callback.ts (2)
65-67: LGTM - Explicit event types handling with defaults.The spread operator correctly creates a mutable copy of
DEFAULT_EVENT_TYPES_ARRAY(which is readonly), preventing accidental mutations.
78-84: LGTM - Refactored to use centralized message formatter.Delegating message formatting to
sendSubscriptionSuccessimproves consistency and reduces duplication.src/services/subscription-service.ts (3)
692-715: LGTM - Clean extraction of subscription success messaging.The method properly handles both webhook and polling modes, uses the new formatters, and registers pending messages for polling mode upgrades.
771-807: LGTM - Clean implementation ofgetSubscription.The method correctly normalizes event types via
parseEventTypesand uses proper query constraints.
1005-1009: I'll help you verify this review comment about the unsafe type cast inparseEventTypes. Let me investigate the codebase to understand the context and validate the concerns.
<function_calls>
<invoke_name>shell
<invoke_description>Search for ALLOWED_EVENT_TYPES and EventType definitions, and examine the parseEventTypes function</invoke_description>
<invoke_content>#!/bin/bashFind ALLOWED_EVENT_TYPES definition
echo "=== Searching for ALLOWED_EVENT_TYPES ==="
rg -n 'ALLOWED_EVENT_TYPES' --type=tsecho -e "\n=== Searching for EventType definition ==="
rg -n 'type EventType|enum EventType' --type=tsecho -e "\n=== Searching for DEFAULT_EVENT_TYPES_ARRAY ==="
rg -n 'DEFAULT_EVENT_TYPES_ARRAY' --type=ts
</invoke_content>
</invoke_function_calls>
- Add exhaustiveness check to handleInvalidOAuthToken for future-proofing - parseEventTypes now filters invalid DB values via ALLOWED_EVENT_TYPES_SET - Extract validateRepoAccessAndGetSubscription helper to deduplicate OAuth token retrieval and repo access validation in addEventTypes and removeEventTypes methods 🤖 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
🧹 Nitpick comments (4)
src/utils/oauth-helpers.ts (1)
115-178: Consider error handling and pattern consistency in token status handling.The function properly uses type narrowing with
Exclude<TokenStatus, TokenStatus.Valid>and an exhaustive check. However, there are two areas worth reviewing:
Inconsistent messaging pattern for
TokenStatus.Unknown: TheNotLinkedandInvalidcases usesendEditableOAuthPrompt(two-phase pattern with message editing), butUnknowndirectly callssendMessage. This means theUnknowncase doesn't benefit from the "Checking..." → final message flow and doesn't track the eventId.Missing error handling: None of the async calls to
sendEditableOAuthPromptorsendMessageare wrapped in try-catch. If these fail, the function will throw and propagate the error to the caller.Option 1: Use consistent pattern for Unknown case
case TokenStatus.Unknown: { - const authUrl = await oauthService.getAuthorizationUrl( + await sendEditableOAuthPrompt( + oauthService, + handler, userId, channelId, spaceId, + `⚠️ **Unable to Verify GitHub Connection**\n\n` + + `We couldn't verify your GitHub token. This could be temporary (rate limiting) or indicate a connection issue.\n\n` + + `Please try again in a few moments, or [reconnect your account]({authUrl}) if the problem persists.`, redirectAction, redirectData ); - await handler.sendMessage( - channelId, - `⚠️ **Unable to Verify GitHub Connection**\n\n` + - `We couldn't verify your GitHub token. This could be temporary (rate limiting) or indicate a connection issue.\n\n` + - `Please try again in a few moments, or [reconnect your account](${authUrl}) if the problem persists.` - ); return; }Option 2: Add error handling
switch (tokenStatus) { case TokenStatus.NotLinked: + try { await sendEditableOAuthPrompt( oauthService, handler, userId, channelId, spaceId, `🔐 **GitHub Account Required**\n\n` + `To modify subscriptions, you need to connect your GitHub account.\n\n` + `[Connect GitHub Account]({authUrl})`, redirectAction, redirectData ); + } catch (error) { + console.error("Failed to send OAuth prompt for NotLinked status:", error); + throw error; + } return;src/services/subscription-service.ts (3)
275-327: Review the early return behavior when newEventTypes is empty.The method correctly validates access, merges, and deduplicates event types. However, the early return at lines 291-293 when
newEventTypesis empty means the subscription'supdatedAttimestamp won't be updated, even though the method was called.This behavior is probably fine, but consider: if a user explicitly tries to add event types that are already present, should the
updatedAtfield be updated to reflect the operation attempt? The current implementation would return success without updating the timestamp.If you want to track all modification attempts, consider removing the early return and allowing the update to proceed even with no net changes:
- if (newEventTypes.length === 0) { - return { success: true, eventTypes: currentTypes }; - } - // Merge and deduplicate const mergedTypes = [ ...new Set([...currentTypes, ...newEventTypes]), ] as EventType[]; + + // No new types to add + if (mergedTypes.length === currentTypes.length) { + return { success: true, eventTypes: currentTypes }; + }This approach deduplicates first, then checks if there's actually anything new to add before updating the database.
809-829: Add error handling for message sending failure.The method correctly formats and sends subscription success messages. However, if
sender.sendMessage()throws an error, the pending message registration won't occur, which could lead to inconsistent state.Consider wrapping the message sending and registration in a try-catch:
async sendSubscriptionSuccess( result: Extract<SubscribeResult, { success: true }>, eventTypes: EventType[], channelId: string, sender: Pick<BotHandler, "sendMessage"> ): Promise<void> { const installUrl = result.deliveryMode === "polling" ? result.installUrl : undefined; const deliveryInfo = formatDeliveryInfo(result.deliveryMode, installUrl); const message = formatSubscriptionSuccess( result.repoFullName, eventTypes, deliveryInfo ); - const { eventId } = await sender.sendMessage(channelId, message); - - if (result.deliveryMode === "polling" && eventId) { - this.registerPendingMessage(channelId, result.repoFullName, eventId); + try { + const { eventId } = await sender.sendMessage(channelId, message); + + if (result.deliveryMode === "polling" && eventId) { + this.registerPendingMessage(channelId, result.repoFullName, eventId); + } + } catch (error) { + console.error( + `Failed to send subscription success message for ${result.repoFullName}:`, + error + ); + throw error; } }
953-989: Consider more specific error messaging for repository validation failures.The helper correctly validates OAuth token, repository access, and subscription existence. However, the error handling at lines 972-977 catches all exceptions from
validateRepository()and returns a generic "You don't have access to this repository" message.While this is simple from a UX perspective, it could be misleading when the actual issue is a network error, rate limiting, or another transient problem rather than an access/permissions issue.
Consider logging the actual error for debugging while keeping the user message generic:
const [owner, repo] = repoFullName.split("/"); try { await validateRepository(githubToken, owner, repo); - } catch { + } catch (error) { + console.error( + `Repository validation failed for ${repoFullName}:`, + error instanceof Error ? error.message : String(error) + ); return { success: false, error: "You don't have access to this repository", }; }Alternatively, you could differentiate between error types (403/404 vs others) to provide more helpful messages, similar to how
createSubscriptionhandles validation errors at lines 177-183.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/subscription-service.ts(11 hunks)src/utils/oauth-helpers.ts(2 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/utils/oauth-helpers.tssrc/services/subscription-service.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/utils/oauth-helpers.tssrc/services/subscription-service.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T03:24:12.463Z
Learnt from: CR
Repo: HereNotThere/bot-github PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:24:12.463Z
Learning: Applies to **/*.ts : Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Applied to files:
src/utils/oauth-helpers.tssrc/services/subscription-service.ts
📚 Learning: 2025-11-25T03:24:12.463Z
Learnt from: CR
Repo: HereNotThere/bot-github PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:24:12.463Z
Learning: Applies to **/*.ts : Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Applied to files:
src/utils/oauth-helpers.ts
🧬 Code graph analysis (1)
src/utils/oauth-helpers.ts (2)
src/types/oauth.ts (1)
RedirectAction(12-12)src/constants.ts (1)
EventType(34-34)
🔇 Additional comments (7)
src/utils/oauth-helpers.ts (1)
3-7: LGTM - Necessary imports for OAuth token handling.The new imports support the token status handling logic in the new
handleInvalidOAuthTokenfunction.src/services/subscription-service.ts (6)
2-2: LGTM - Necessary imports for subscription management.The new imports for
BotHandler, constants, and message formatters support the new subscription management features.Also applies to: 12-13, 22-25
335-406: LGTM - Correct implementation of granular event type removal.The method properly handles the complete flow:
- Validates repository access before modifications
- Correctly removes specified event types from the current set
- Automatically deletes the subscription when all event types are removed
- Handles edge cases (subscription no longer exists, delete failures)
The implementation silently ignores event types in
typesToRemovethat aren't currently subscribed, which is reasonable behavior.
501-534: LGTM - Clean implementation of subscription retrieval.The method correctly retrieves a specific subscription and parses the event types using the new
parseEventTypeshelper. The null return for non-existent subscriptions is appropriate.
888-912: LGTM - Clean extraction of installation failure logic.The helper properly consolidates the logic for handling cases where GitHub App installation is required, including storing the pending subscription for later completion.
918-946: LGTM - Correct implementation of pending subscription storage.The helper properly stores pending subscriptions with expiration times and uses
onConflictDoNothing()for idempotent behavior.
992-998: LGTM - Robust event type parsing with validation.The
parseEventTypesutility correctly:
- Returns default event types for null/empty input
- Filters out invalid event types using the allowed set
- Maintains type safety with the
EventType[]return typeThis centralized parsing ensures consistency across all subscription retrieval paths.
shuhuiluo
deleted the
claude/granular-unsubscribe-management-01HS4mvDjHAGGfVjT9VqvP3o
branch
Add support for granular event type management in GitHub subscriptions:
getSubscription()method to retrieve specific subscription detailsaddEventTypes()method to add event types to existing subscriptionsremoveEventTypes()method to remove specific event types/github subscribeto add event types when already subscribed/github unsubscribeto support--eventsflag for granular removalThis allows users to manage subscriptions without removing the entire repository subscription. Users can now:
/github subscribe owner/repo --events stars,forks/github unsubscribe owner/repo --events commitsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.