diff --git a/src/constants.ts b/src/constants.ts index 9889965..ebc4821 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -1,8 +1,15 @@ +/** Default event types as array for code layer */ +export const DEFAULT_EVENT_TYPES_ARRAY: readonly EventType[] = [ + "pr", + "issues", + "commits", + "releases", +]; + /** - * Default event types for GitHub subscriptions - * This constant is imported and used across the codebase to ensure consistency + * Default event types for GitHub subscriptions (string for DB storage) */ -export const DEFAULT_EVENT_TYPES = "pr,issues,commits,releases"; +export const DEFAULT_EVENT_TYPES = DEFAULT_EVENT_TYPES_ARRAY.join(","); /** * Allowed event types that users can subscribe to @@ -26,6 +33,11 @@ export const ALLOWED_EVENT_TYPES = [ */ export type EventType = (typeof ALLOWED_EVENT_TYPES)[number]; +/** Pre-allocated Set for O(1) event type validation */ +export const ALLOWED_EVENT_TYPES_SET: ReadonlySet = new Set( + ALLOWED_EVENT_TYPES +); + /** * Pending message cleanup interval (30 seconds) * How often to check for and remove stale pending messages diff --git a/src/handlers/github-subscription-handler.ts b/src/handlers/github-subscription-handler.ts index 8f8fc82..8acd1ce 100644 --- a/src/handlers/github-subscription-handler.ts +++ b/src/handlers/github-subscription-handler.ts @@ -1,6 +1,12 @@ import type { BotHandler } from "@towns-protocol/bot"; -import { ALLOWED_EVENT_TYPES, DEFAULT_EVENT_TYPES } from "../constants"; +import { + ALLOWED_EVENT_TYPES, + ALLOWED_EVENT_TYPES_SET, + DEFAULT_EVENT_TYPES, + DEFAULT_EVENT_TYPES_ARRAY, + type EventType, +} from "../constants"; import { TokenStatus, type GitHubOAuthService, @@ -91,7 +97,7 @@ async function handleSubscribe( } // Parse and validate event types from args - let eventTypes: string; + let eventTypes: EventType[]; try { eventTypes = parseEventTypes(args); } catch (error) { @@ -318,12 +324,12 @@ async function handleStatus( /** * Parse and validate event types from --events flag - * Returns default types if no flag, or comma-separated validated event types + * Returns default types if no flag, or validated event types array * @throws Error if any event type is invalid */ -function parseEventTypes(args: string[]): string { +function parseEventTypes(args: string[]): EventType[] { const eventsIndex = args.findIndex(arg => arg.startsWith("--events")); - if (eventsIndex === -1) return DEFAULT_EVENT_TYPES; + if (eventsIndex === -1) return [...DEFAULT_EVENT_TYPES_ARRAY]; let rawEventTypes: string; @@ -334,7 +340,7 @@ function parseEventTypes(args: string[]): string { // Check for --events pr,issues format (next arg) rawEventTypes = args[eventsIndex + 1]; } else { - return DEFAULT_EVENT_TYPES; + return [...DEFAULT_EVENT_TYPES_ARRAY]; } // Parse and validate event types @@ -345,14 +351,13 @@ function parseEventTypes(args: string[]): string { // Handle "all" as special case if (tokens.includes("all")) { - return ALLOWED_EVENT_TYPES.join(","); + return [...ALLOWED_EVENT_TYPES]; } // Validate each token const invalidTokens: string[] = []; - const allowedSet = new Set(ALLOWED_EVENT_TYPES); for (const token of tokens) { - if (!allowedSet.has(token as (typeof ALLOWED_EVENT_TYPES)[number])) { + if (!ALLOWED_EVENT_TYPES_SET.has(token as EventType)) { invalidTokens.push(token); } } @@ -366,17 +371,13 @@ function parseEventTypes(args: string[]): string { ); } - // Remove duplicates using Set and return - const uniqueTokens = Array.from(new Set(tokens)); - return uniqueTokens.join(","); + // Remove duplicates and return + return [...new Set(tokens)] as EventType[]; } /** * Format event types for display */ -function formatEventTypes(eventTypes: string): string { - return eventTypes - .split(",") - .map(t => t.trim()) - .join(", "); +function formatEventTypes(eventTypes: EventType[]): string { + return eventTypes.join(", "); } diff --git a/src/routes/oauth-callback.ts b/src/routes/oauth-callback.ts index 768e77c..c27cf80 100644 --- a/src/routes/oauth-callback.ts +++ b/src/routes/oauth-callback.ts @@ -1,7 +1,7 @@ import type { Context } from "hono"; import { getOwnerIdFromUsername, parseRepo } from "../api/github-client"; -import { DEFAULT_EVENT_TYPES } from "../constants"; +import { DEFAULT_EVENT_TYPES_ARRAY } from "../constants"; import { generateInstallUrl, type InstallationService, @@ -70,7 +70,9 @@ export async function handleOAuthCallback( spaceId: result.spaceId, channelId: result.channelId, repoIdentifier: result.redirectData.repo, - eventTypes: result.redirectData.eventTypes || DEFAULT_EVENT_TYPES, + eventTypes: result.redirectData.eventTypes ?? [ + ...DEFAULT_EVENT_TYPES_ARRAY, + ], }); if (subResult.success) { diff --git a/src/services/polling-service.ts b/src/services/polling-service.ts index 3cef04a..5459a22 100644 --- a/src/services/polling-service.ts +++ b/src/services/polling-service.ts @@ -329,22 +329,18 @@ export class PollingService { /** * Check if an event matches the subscription's event type filter * @param eventType - GitHub event type (e.g., "PullRequestEvent") or null - * @param subscriptionTypes - Comma-separated event types (e.g., "pr,issues") or "all" or null + * @param subscriptionTypes - Array of event types (e.g., ["pr", "issues"]) */ function isEventTypeMatch( eventType: string | null, - subscriptionTypes: string | null | undefined + subscriptionTypes: EventType[] ): boolean { - // Treat null event type or null/undefined/"all" subscription as match - if (!eventType || !subscriptionTypes || subscriptionTypes === "all") - return true; - - const subscribedTypes = subscriptionTypes.split(",").map(t => t.trim()); + // Treat null event type or empty subscription as match-all + if (!eventType || subscriptionTypes.length === 0) return true; // Check if the event type matches any of the subscribed short names - for (const shortName of subscribedTypes) { - const mappedTypes = - EVENT_TYPE_MAP[shortName as EventType]?.split(",") ?? []; + for (const shortName of subscriptionTypes) { + const mappedTypes = EVENT_TYPE_MAP[shortName]?.split(",") ?? []; if (mappedTypes.includes(eventType)) { return true; } diff --git a/src/services/subscription-service.ts b/src/services/subscription-service.ts index 793f56c..4144615 100644 --- a/src/services/subscription-service.ts +++ b/src/services/subscription-service.ts @@ -13,6 +13,7 @@ import { PENDING_MESSAGE_MAX_AGE_MS, PENDING_SUBSCRIPTION_CLEANUP_INTERVAL_MS, PENDING_SUBSCRIPTION_EXPIRATION_MS, + type EventType, } from "../constants"; import { db } from "../db"; import { githubSubscriptions, pendingSubscriptions } from "../db/schema"; @@ -31,7 +32,7 @@ export interface SubscribeParams { spaceId: string; channelId: string; repoIdentifier: string; // Format: "owner/repo" - eventTypes?: string; + eventTypes: EventType[]; } /** @@ -44,13 +45,13 @@ type SubscribeSuccess = success: true; deliveryMode: "webhook"; repoFullName: string; - eventTypes: string; + eventTypes: EventType[]; } | { success: true; deliveryMode: "polling"; repoFullName: string; - eventTypes: string; + eventTypes: EventType[]; installUrl: string; }; @@ -61,7 +62,7 @@ type SubscribeFailure = requiresInstallation: true; installUrl: string; repoFullName: string; - eventTypes: string; + eventTypes: EventType[]; error: string; }; @@ -106,9 +107,6 @@ export class SubscriptionService { const { townsUserId, spaceId, channelId, repoIdentifier, eventTypes } = params; - // Normalize event types once to avoid duplication - const requestedEventTypes = eventTypes || DEFAULT_EVENT_TYPES; - // Parse owner/repo const [owner, repo] = repoIdentifier.split("/"); if (!owner || !repo) { @@ -160,23 +158,14 @@ export class SubscriptionService { // Get owner ID from public profile for installation URL const ownerId = await getOwnerIdFromUsername(owner); - // Store pending subscription for completion after installation - await this.storePendingSubscription({ + return this.requiresInstallationFailure({ townsUserId, spaceId, channelId, repoFullName: repoIdentifier, - eventTypes: requestedEventTypes, + eventTypes, + ownerId, }); - - return { - success: false, - requiresInstallation: true, - installUrl: generateInstallUrl(ownerId), - repoFullName: repoIdentifier, - eventTypes: requestedEventTypes, - error: `Repository not found or you don't have access.`, - }; } // Check if this might be an org approval issue @@ -207,23 +196,14 @@ export class SubscriptionService { if (repoInfo.isPrivate) { // Private repos MUST have GitHub App installed if (!installationId) { - // Store pending subscription for completion after installation - await this.storePendingSubscription({ + return this.requiresInstallationFailure({ townsUserId, spaceId, channelId, repoFullName: repoInfo.fullName, - eventTypes: requestedEventTypes, + eventTypes, + ownerId: repoInfo.owner.id, }); - - return { - success: false, - requiresInstallation: true, - installUrl: generateInstallUrl(repoInfo.owner.id), - repoFullName: repoInfo.fullName, - eventTypes: requestedEventTypes, - error: `Private repository requires GitHub App installation`, - }; } deliveryMode = "webhook"; @@ -265,7 +245,7 @@ export class SubscriptionService { createdByGithubLogin: githubUser.login, installationId, enabled: true, - eventTypes: requestedEventTypes, + eventTypes: eventTypes.join(","), createdAt: now, updatedAt: now, }); @@ -275,7 +255,7 @@ export class SubscriptionService { success: true, deliveryMode: "polling", repoFullName: repoInfo.fullName, - eventTypes: requestedEventTypes, + eventTypes, installUrl: generateInstallUrl(repoInfo.owner.id), }; } else { @@ -283,11 +263,40 @@ export class SubscriptionService { success: true, deliveryMode: "webhook", repoFullName: repoInfo.fullName, - eventTypes: requestedEventTypes, + eventTypes, }; } } + /** + * Store pending subscription and return failure requiring installation + */ + private async requiresInstallationFailure(params: { + townsUserId: string; + spaceId: string; + channelId: string; + repoFullName: string; + eventTypes: EventType[]; + ownerId?: number; + }): Promise { + await this.storePendingSubscription({ + townsUserId: params.townsUserId, + spaceId: params.spaceId, + channelId: params.channelId, + repoFullName: params.repoFullName, + eventTypes: params.eventTypes, + }); + + return { + success: false, + requiresInstallation: true, + installUrl: generateInstallUrl(params.ownerId), + repoFullName: params.repoFullName, + eventTypes: params.eventTypes, + error: "Private repository requires GitHub App installation", + }; + } + /** * Store a pending subscription for completion after GitHub App installation * Used when user tries to subscribe to private repo before installing GitHub App @@ -297,7 +306,7 @@ export class SubscriptionService { spaceId: string; channelId: string; repoFullName: string; - eventTypes: string; + eventTypes: EventType[]; }): Promise { const now = new Date(); const expiresAt = new Date( @@ -311,7 +320,7 @@ export class SubscriptionService { spaceId: params.spaceId, channelId: params.channelId, repoFullName: params.repoFullName, - eventTypes: params.eventTypes, + eventTypes: params.eventTypes.join(","), createdAt: now, expiresAt, }) @@ -360,7 +369,7 @@ export class SubscriptionService { spaceId: sub.spaceId, channelId: sub.channelId, repoIdentifier: repoFullName, - eventTypes: sub.eventTypes, + eventTypes: sub.eventTypes.split(",") as EventType[], }); if (result.success && this.bot) { @@ -419,7 +428,7 @@ export class SubscriptionService { channelId: string, spaceId: string ): Promise< - Array<{ repo: string; eventTypes: string; deliveryMode: string }> + Array<{ repo: string; eventTypes: EventType[]; deliveryMode: string }> > { const results = await db .select({ @@ -437,7 +446,9 @@ export class SubscriptionService { return results.map(r => ({ repo: r.repo, - eventTypes: r.eventTypes || DEFAULT_EVENT_TYPES, + eventTypes: (r.eventTypes || DEFAULT_EVENT_TYPES).split( + "," + ) as EventType[], deliveryMode: r.deliveryMode, })); } @@ -453,7 +464,7 @@ export class SubscriptionService { async getRepoSubscribers( repoFullName: string, deliveryMode?: "webhook" | "polling" - ): Promise> { + ): Promise> { const conditions = [eq(githubSubscriptions.repoFullName, repoFullName)]; if (deliveryMode) { @@ -470,7 +481,9 @@ export class SubscriptionService { return results.map(r => ({ channelId: r.channelId, - eventTypes: r.eventTypes || DEFAULT_EVENT_TYPES, + eventTypes: (r.eventTypes || DEFAULT_EVENT_TYPES).split( + "," + ) as EventType[], })); } diff --git a/src/types/oauth.ts b/src/types/oauth.ts index 17a41ce..ed25265 100644 --- a/src/types/oauth.ts +++ b/src/types/oauth.ts @@ -1,13 +1,18 @@ import { z } from "zod"; +import { ALLOWED_EVENT_TYPES } from "../constants"; + /** Supported redirect actions after OAuth completion */ export const RedirectActionSchema = z.enum(["subscribe", "query"]); export type RedirectAction = z.infer; +/** Event type schema for validation */ +export const EventTypeSchema = z.enum(ALLOWED_EVENT_TYPES); + /** Redirect data passed through OAuth state */ export const RedirectDataSchema = z.object({ repo: z.string(), - eventTypes: z.string().optional(), + eventTypes: z.array(EventTypeSchema).optional(), messageEventId: z.string().optional(), }); export type RedirectData = z.infer; diff --git a/src/views/oauth-pages.ts b/src/views/oauth-pages.ts index 4396397..6fa5e41 100644 --- a/src/views/oauth-pages.ts +++ b/src/views/oauth-pages.ts @@ -188,7 +188,7 @@ function renderWebhookSuccess( sub: Extract ) { const safeRepo = escapeHtml(sub.repoFullName); - const safeEvents = escapeHtml(sub.eventTypes); + const safeEvents = escapeHtml(sub.eventTypes.join(", ")); return c.html(` @@ -206,7 +206,7 @@ function renderWebhookSuccess(

✅ Subscribed to ${safeRepo}!

⚡ Real-time webhook delivery enabled

-

Events: ${safeEvents.replace(/,/g, ", ")}

+

Events: ${safeEvents}

You can close this window and return to Towns.

@@ -222,7 +222,7 @@ function renderPollingSuccess( sub: Extract ) { const safeRepo = escapeHtml(sub.repoFullName); - const safeEvents = escapeHtml(sub.eventTypes); + const safeEvents = escapeHtml(sub.eventTypes.join(", ")); const safeInstallUrl = escapeHtml(sub.installUrl); return c.html(` @@ -241,7 +241,7 @@ function renderPollingSuccess(

✅ Subscribed to ${safeRepo}!

⏱️ Currently using 5-minute polling

-

Events: ${safeEvents.replace(/,/g, ", ")}

+

Events: ${safeEvents}

💡 Want real-time updates?

Install GitHub App diff --git a/tests/unit/handlers/github-subscription-handler.test.ts b/tests/unit/handlers/github-subscription-handler.test.ts index 44a3575..aa9cfc6 100644 --- a/tests/unit/handlers/github-subscription-handler.test.ts +++ b/tests/unit/handlers/github-subscription-handler.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, expect, mock, test } from "bun:test"; import { ALLOWED_EVENT_TYPES, DEFAULT_EVENT_TYPES, + type EventType, } from "../../../src/constants"; import { handleGithubSubscription } from "../../../src/handlers/github-subscription-handler"; import { TokenStatus } from "../../../src/services/github-oauth-service"; @@ -121,7 +122,11 @@ describe("github subscription handler", () => { test("should handle case-insensitive actions - status", async () => { mockSubscriptionService.getChannelSubscriptions = mock(() => Promise.resolve([ - { repo: "owner/repo", eventTypes: DEFAULT_EVENT_TYPES }, + { + repo: "owner/repo", + eventTypes: DEFAULT_EVENT_TYPES.split(",") as EventType[], + deliveryMode: "webhook", + }, ]) ); @@ -193,7 +198,7 @@ describe("github subscription handler", () => { spaceId: "test-space", channelId: "test-channel", repoIdentifier: "facebook/react", - eventTypes: DEFAULT_EVENT_TYPES, + eventTypes: DEFAULT_EVENT_TYPES.split(","), }); const message = mockHandler.sendMessage.mock.calls[0][1]; @@ -211,7 +216,7 @@ describe("github subscription handler", () => { ); const createCalls = mockSubscriptionService.createSubscription.mock.calls; - expect(createCalls[0][0].eventTypes).toBe("pr,ci"); + expect(createCalls[0][0].eventTypes).toEqual(["pr", "ci"]); }); test("should handle --events=all flag", async () => { @@ -225,7 +230,7 @@ describe("github subscription handler", () => { ); const createCalls = mockSubscriptionService.createSubscription.mock.calls; - expect(createCalls[0][0].eventTypes).toBe(ALLOWED_EVENT_TYPES.join(",")); + expect(createCalls[0][0].eventTypes).toEqual([...ALLOWED_EVENT_TYPES]); }); test("should reject invalid event types", async () => { @@ -564,12 +569,12 @@ describe("github subscription handler", () => { Promise.resolve([ { repo: "owner/repo1", - eventTypes: DEFAULT_EVENT_TYPES, + eventTypes: DEFAULT_EVENT_TYPES.split(",") as EventType[], deliveryMode: "webhook", }, { repo: "owner/repo2", - eventTypes: "pr,ci", + eventTypes: ["pr", "ci"] as EventType[], deliveryMode: "polling", }, ])