refactor: extract subscription handler switch cases to functions

[shuhuiluo]

Extract switch case logic into dedicated handler functions for better organization and maintainability:

• handleSubscribe() - handles repository subscription with OAuth and installation flow
• handleUnsubscribe() - handles repository unsubscription
• handleStatus() - displays channel subscriptions

Also added ephemeral OAuth URLs (security fix) to prevent account hijacking vulnerability.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added unified GitHub subscription command with subscribe, unsubscribe and status actions.

• Improvements

  • Better validation and consistent error/usage messages across actions.
  • Clearer OAuth and GitHub App installation prompts for private repo flows.
  • Status output now shows per-repo delivery mode and event types.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR replaces a monolithic GitHub subscription handler with a modular implementation in src/handlers/github-subscription-handler.ts. It exports a public handleGithubSubscription(handler, event, subscriptionService) that dispatches to internal handlers for subscribe, unsubscribe, and status. New helpers parseEventTypes and formatEventTypes are added; subscribe/unsubscribe/status flows are implemented with repo validation, OAuth/initiation for installs, GitHub App installation handling for private repos, and unified usage/error messaging.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Single file with multiple distinct logic paths (subscribe, unsubscribe, status) requiring separate verification.
• OAuth and GitHub App installation flows — check authentication/error paths and external API interactions.
• Formatting and parsing helpers (parseEventTypes/formatEventTypes) and message construction should be validated for edge cases.

Files/areas to pay extra attention:

• src/handlers/github-subscription-handler.ts (core logic branches and user-facing messages)
• SubscriptionService interactions and assumptions about its API surface
• Repo validation and GitHub App installation/error handling paths

Possibly related PRs

• feat: implement OAuth-first subscription service with dual-mode delivery #32 — Refactors the subscription handler to delegate to a SubscriptionService; directly related to this change.
• feat: subscription feature for PRs and issues #18 — Another change to subscribe/unsubscribe/status flows in the same handler, representing a related implementation.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main refactoring work: extracting switch case logic into dedicated handler functions, which aligns with the primary changes in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/subscription-handler-functions

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/handlers/github-subscription-handler.ts (3)

55-163: Subscribe handler logic and OAuth/installation flows look solid

• Repo handling is careful: you strip markdown (stripMarkdown) and validate owner/repo shape before touching the service, which avoids garbage going into SubscriptionService.
• Event-type parsing is centralized via parseEventTypes(args) and properly wrapped in a try/catch, so invalid flags yield a clear, single error message to the user instead of throwing.
• The OAuth requirement branch sends the auth URL via handler.sendMessage(..., { ephemeral: true }), which matches the PR goal of preventing OAuth URL leakage in the channel.
• Private-repo installation requirements and public-repo “suggest install for webhooks” paths are surfaced with detailed, actionable copy; success copy includes delivery mode and the GitHub link.

One minor behavioral nit you might consider (optional): if a user supplies --events without any value, parseEventTypes falls back to DEFAULT_EVENT_TYPES rather than treating it as an error, which may hide a typo in the command. If you want stricter UX, you could detect --events with no value and throw instead.

If you want to double-check the ephemeral behavior of handler.sendMessage, you can confirm in @towns-protocol/bot docs that { ephemeral: true } indeed restricts visibility to the invoking user only.

---

238-269: Status handler reads well; consider future-proofing for very long lists (optional)

The status handler correctly:

• Fetches channel subscriptions once,
• Returns a friendly “no subscriptions” message, and
• Renders each repo with a mode icon and formatted event types via formatEventTypes.

For typical channel sizes, this is great. If you anticipate channels subscribing to many dozens of repos, you might later want to paginate or truncate repoList to avoid overly long messages or platform max-length issues, but that’s an optimization you can defer.

---

276-324: Event-type parsing is robust; minor edge cases around all and empty values

The parseEventTypes helper is nicely self-contained and handles:

• Both --events=pr,issues and --events pr,issues syntax,
• Defaults when the flag is absent,
• Normalization to lowercase with trimming,
• Validation against ALLOWED_EVENT_TYPES, and
• Expansion of all to the full set.

Two small behavioral notes (optional, not blockers):

1. all plus typos is silently accepted.
   --events=foo,all,bar results in tokens.includes("all") and immediately returns ALLOWED_EVENT_TYPES.join(","), skipping validation of foo/bar. If you’d rather fail on typos when all is combined with other values, validate first and only treat all specially if it’s the only token.

2. Bare --events falls back to default.
   As with the subscribe handler comment, --events without a value returns DEFAULT_EVENT_TYPES. That’s defensible, but if you want tighter UX you could instead throw an error to signal a malformed flag.

If you’re happy with the current semantics, the implementation is otherwise clear and performant.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc88b3a and 7b50504.

📒 Files selected for processing (1)

• src/handlers/github-subscription-handler.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/handlers/github-subscription-handler.ts (3)

src/types/bot.ts (1)

• SlashCommandEvent (13-15)

src/services/subscription-service.ts (1)

• SubscriptionService (47-356)

src/utils/stripper.ts (1)

• stripMarkdown (6-13)

🔇 Additional comments (2)

src/handlers/github-subscription-handler.ts (2)

11-50: Top-level dispatcher is clear and keeps public API small

The main handleGithubSubscription function cleanly parses action, provides a helpful usage message when missing, and routes to the three internal handlers with minimal branching. The unknown-action message is explicit and constrained to the supported verbs, which is good for UX. No issues from a correctness or security perspective here.

---

329-333: Event-type display formatter is simple and sufficient

formatEventTypes just normalizes spacing around commas, which is exactly what’s needed for user-facing messages and keeps behavior predictable across handlers. No issues here.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/handlers/github-subscription-handler.ts (3)

19-27: Deduplicate usage/help strings to avoid drift

Usage/help text for /github (full usage, subscribe, unsubscribe, status) is hard-coded in several places. This is easy to let drift from the actual behavior or allowed event types.

Consider extracting a shared const GITHUB_USAGE = ... (or a small helper) and, ideally, deriving the event-types portion from ALLOWED_EVENT_TYPES so adding/removing types in the constants automatically updates the help text.

Also applies to: 63-69, 176-181, 254-257

---

63-82: Tighten repo sanitization and handle empty --events edge case

Two small robustness tweaks worth considering:

1. Repo sanitization:
   stripMarkdown removes bold/italic/code markers but not angle brackets or leading/trailing whitespace. Inputs like <owner/repo> or " owner/repo " will pass the / check but produce invalid owner/repo segments for subscribeToRepository. A small normalization step would help:

-  const repo = stripMarkdown(repoArg);
+  const repo = stripMarkdown(repoArg).trim().replace(/^<|>$/g, "");

2. Empty --events tokens:
   For --events , or --events=,, tokens ends up empty and parseEventTypes currently returns "", which gets stored as an empty string while other paths treat falsy values as “use defaults”. You can normalize this by short‑circuiting when tokens.length === 0:

  const tokens = rawEventTypes
    .split(",")
    .map(token => token.trim().toLowerCase())
    .filter(token => token.length > 0);

+ if (tokens.length === 0) {
+   return DEFAULT_EVENT_TYPES;
+ }

This keeps DB state consistent with the runtime behavior.

Also applies to: 83-93, 280-328

---

249-273: Optional: sort status output for readability

handleStatus correctly lists all channel subscriptions, including delivery mode and event types. For channels with many repos, the list might be easier to scan if it were sorted (e.g., alphabetically by sub.repo or grouped by deliveryMode):

-  const repoList = subscriptions
-    .map(sub => {
+  const repoList = [...subscriptions]
+    .sort((a, b) => a.repo.localeCompare(b.repo))
+    .map(sub => {
       const mode = sub.deliveryMode === "webhook" ? "⚡" : "⏱️";
       return `${mode} ${sub.repo} (${formatEventTypes(sub.eventTypes)})`;
     })

Not required, but could improve UX in busy channels.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b50504 and 495912a.

📒 Files selected for processing (1)

• src/handlers/github-subscription-handler.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/handlers/github-subscription-handler.ts (3)

src/types/bot.ts (1)

• SlashCommandEvent (13-15)

src/services/subscription-service.ts (1)

• SubscriptionService (47-356)

src/utils/stripper.ts (1)

• stripMarkdown (6-13)

🔇 Additional comments (2)

src/handlers/github-subscription-handler.ts (2)

103-113: Ephemeral OAuth auth URL handling looks solid

Using { ephemeral: true } for the OAuth “Connect GitHub Account” message ensures the authorization URL is only visible to the requesting user, which directly mitigates link-leakage/account-hijack style issues described in the PR. No further changes needed here.

---

196-227: Unsubscribe now correctly handles repo casing and canonical names

The updated unsubscribe flow:

• Checks channel subscriptions up front.
• Locates the subscription with a case-insensitive match (sub.repo.toLowerCase() === repo.toLowerCase()).
• Calls subscriptionService.unsubscribe with subscription.repo (the canonical DB value) while still echoing the user’s original repo in messages.

This resolves the prior case-sensitivity issue and guarantees the correct row is targeted without impacting UX.

Also applies to: 229-237