diff --git a/backend/.env.example b/backend/.env.example index 1d5f860..f48365b 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -104,3 +104,13 @@ POSTHOG_HOST=https://eu.i.posthog.com # 0 turns the sweep off entirely without a release, which is also what the e2e suite runs with: # a timer reaching for GitHub in the background would make every spec's mocks a race. INBOX_WARM_SWEEP_INTERVAL_MS=300000 + +# --- Daily digest --- +# How often the sweep looks for somebody whose chosen hour has come, in milliseconds. Not how +# often anybody is messaged: one digest per person per local day is settled by a conditional +# write whatever this is set to, so it only decides how soon after their hour they hear. +# +# A quarter hour is the coarsest that still honours zones offset by half and three-quarter hours. +# 0 turns it off entirely without a release, which is what the e2e suite runs with - and more so +# than the sweep above, because this timer posts to Slack rather than only reading. +DIGEST_SWEEP_INTERVAL_MS=900000 diff --git a/backend/src/analytics/analytics-events.ts b/backend/src/analytics/analytics-events.ts index 94fe5cb..77e8f41 100644 --- a/backend/src/analytics/analytics-events.ts +++ b/backend/src/analytics/analytics-events.ts @@ -83,4 +83,4 @@ export type AnalyticsEvent = * the dashboard's test button and the message that proves a fresh connection works both go out * the same pipe, and both are worth telling apart from a real poke rather than hiding. */ -export type PokeTrigger = 'github_webhook' | 'test' | 'welcome'; +export type PokeTrigger = 'github_webhook' | 'test' | 'welcome' | 'digest'; diff --git a/backend/src/analytics/metrics-catalog.ts b/backend/src/analytics/metrics-catalog.ts index 4bbf6af..ba02f79 100644 --- a/backend/src/analytics/metrics-catalog.ts +++ b/backend/src/analytics/metrics-catalog.ts @@ -47,7 +47,9 @@ export type CounterName = | 'proke.poke.delivered' | 'proke.cache.lookups' | 'proke.inbox.warmed' - | 'proke.inbox.warm.sweeps'; + | 'proke.inbox.warm.sweeps' + | 'proke.digest.sent' + | 'proke.digest.sweeps'; /** Gauges. A value that moves both ways, read at a moment. */ export type GaugeName = 'proke.event_loop.delay'; @@ -61,6 +63,7 @@ export type GaugeName = 'proke.event_loop.delay'; */ export type HistogramName = | 'proke.inbox.warm.duration' + | 'proke.digest.duration' | 'proke.webhook.duration' | 'proke.poke.latency' | 'proke.github.request.duration' @@ -151,6 +154,22 @@ export type WarmOutcome = 'refreshed' | 'no_token' | 'github_unavailable' | 'fai */ export type WarmSweepOutcome = 'completed' | 'overlapped' | 'failed'; +/** + * What one person's digest came to. + * + * `empty` is the healthy majority rather than a failure, and kept apart from `sent` because the + * ratio between them says whether the digest is worth sending at all. `claimed_already` on a + * single-instance deploy would mean the claim is not doing its job. + */ +export type DigestOutcome = + | 'sent' + | 'empty' + | 'claimed_already' + | 'no_token' + | 'github_unavailable' + | 'undeliverable' + | 'failed'; + /** * Which GitHub call this was. Hand-written labels rather than URLs, which carry ids. * @@ -250,6 +269,9 @@ export interface MetricAttributeMap { // Undimensioned: there is one sweep, and splitting it by anything would only make the series // smaller without making it answer a different question. 'proke.inbox.warm.duration': Record; + 'proke.digest.sent': { outcome: DigestOutcome }; + 'proke.digest.sweeps': { outcome: WarmSweepOutcome }; + 'proke.digest.duration': Record; 'proke.event_loop.delay': { quantile: EventLoopQuantile }; 'http.server.duration': { route: string; method: string; status: string }; } diff --git a/backend/src/app.module.ts b/backend/src/app.module.ts index 4721ccc..948f7f6 100644 --- a/backend/src/app.module.ts +++ b/backend/src/app.module.ts @@ -5,6 +5,7 @@ import { AuthCoreModule } from './auth/core/auth-core.module'; import { ConnectionsModule } from './connections/connections.module'; import { InboxModule } from './inbox/inbox.module'; import { InboxWarmModule } from './inbox/warm/inbox-warm.module'; +import { DigestModule } from './notifications/digest/digest.module'; import { PokeSettingsModule } from './notifications/settings/poke-settings.module'; import { getEnvConfig } from './shared/configs/env-configs'; import { HttpMetricsModule } from './shared/http/http-metrics.middleware'; @@ -30,6 +31,8 @@ import { SlackEventsModule } from './webhooks/slack/slack-events.module'; // What kinds of poke somebody wants, account-wide. The delivery side of notifications is // wired in under the webhook module; this is only the settings route. PokeSettingsModule, + // The other scheduler: the daily list of what is still waiting on somebody. + DigestModule, SlackModule, GithubWebhookModule, SlackEventsModule, diff --git a/backend/src/inbox/core/entities/inbox.interface.ts b/backend/src/inbox/core/entities/inbox.interface.ts index 5dcec29..e3cb7fe 100644 --- a/backend/src/inbox/core/entities/inbox.interface.ts +++ b/backend/src/inbox/core/entities/inbox.interface.ts @@ -60,6 +60,9 @@ export interface InboxPullRequest { title: string; url: string; isDraft: boolean; + /** ISO 8601, as GitHub gave it. When the pull request was opened, not when it last moved. */ + createdAt: string; + changedFiles: number; repositoryId: string; repositoryFullName: string; author: InboxAuthor; diff --git a/backend/src/inbox/dto/inbox.response.ts b/backend/src/inbox/dto/inbox.response.ts index f45f817..27ddea9 100644 --- a/backend/src/inbox/dto/inbox.response.ts +++ b/backend/src/inbox/dto/inbox.response.ts @@ -25,6 +25,12 @@ export class InboxPullRequestResponse { @ApiProperty() isDraft: boolean; + @ApiProperty({ description: 'ISO 8601. When the pull request was opened, not when it moved.' }) + createdAt: string; + + @ApiProperty({ description: 'How many files it touches.' }) + changedFiles: number; + @ApiProperty({ description: "GitHub's numeric repository id, as a string" }) repositoryId: string; diff --git a/backend/src/inbox/github-inbox-data.service.ts b/backend/src/inbox/github-inbox-data.service.ts index 41b70ea..c4ba71d 100644 --- a/backend/src/inbox/github-inbox-data.service.ts +++ b/backend/src/inbox/github-inbox-data.service.ts @@ -24,6 +24,12 @@ export interface GithubInboxPullRequest { * and it answers the question the ordering is actually asking - what moved most recently. */ updatedAt: string; + /** + * When it was opened, and the clock the digest ages rows by. Not `updatedAt`, which moves on + * every comment, so the row argued about hardest would read as the newest. + */ + createdAt: string; + changedFiles: number; repositoryId: string; repositoryFullName: string; authorLogin: string; @@ -91,17 +97,22 @@ export class GithubInboxDataService { let response: Response; try { - response = await githubFetch(this.metrics, 'graphql_inbox', 'https://api.github.com/graphql', { - method: 'POST', - headers: { - Authorization: `Bearer ${accessToken}`, - 'Content-Type': 'application/json', + response = await githubFetch( + this.metrics, + 'graphql_inbox', + 'https://api.github.com/graphql', + { + method: 'POST', + headers: { + Authorization: `Bearer ${accessToken}`, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ + query: INBOX_QUERY, + variables: { yours: MAX_YOURS, waiting: MAX_WAITING, threads: MAX_THREADS }, + }), }, - body: JSON.stringify({ - query: INBOX_QUERY, - variables: { yours: MAX_YOURS, waiting: MAX_WAITING, threads: MAX_THREADS }, - }), - }); + ); } catch (error) { this.logger.warn(`Could not reach GitHub for an inbox: ${error}`); return null; @@ -159,6 +170,8 @@ function normalizeSearch(search: any): GithubInboxPullRequest[] { url: node.url ?? '', isDraft: Boolean(node.isDraft), updatedAt: node.updatedAt ?? '', + createdAt: node.createdAt ?? '', + changedFiles: Number(node.changedFiles ?? 0), repositoryId: node.repository.id, repositoryFullName: node.repository.nameWithOwner, authorLogin: login, @@ -231,6 +244,8 @@ fragment Row on PullRequest { url isDraft updatedAt + createdAt + changedFiles reviewDecision repository { id nameWithOwner } author { __typename login avatarUrl } diff --git a/backend/src/inbox/inbox-classifier.ts b/backend/src/inbox/inbox-classifier.ts index 19c6d81..12579ca 100644 --- a/backend/src/inbox/inbox-classifier.ts +++ b/backend/src/inbox/inbox-classifier.ts @@ -1,7 +1,4 @@ -import { - GithubInboxPullRequest, - GithubInbox, -} from './github-inbox-data.service'; +import { GithubInboxPullRequest, GithubInbox } from './github-inbox-data.service'; import { InboxBuildFilters, InboxViewFilters, @@ -161,6 +158,8 @@ function toPullRequest(pullRequest: GithubInboxPullRequest): InboxPullRequest { title: pullRequest.title, url: pullRequest.url, isDraft: pullRequest.isDraft, + createdAt: pullRequest.createdAt, + changedFiles: pullRequest.changedFiles, repositoryId: pullRequest.repositoryId, repositoryFullName: pullRequest.repositoryFullName, author: { @@ -224,9 +223,7 @@ function group( keys: readonly InboxSectionKey[], sectionOf: (pullRequest: GithubInboxPullRequest) => InboxSectionKey, ): InboxSectionContent[] { - const buckets = new Map( - keys.map((key) => [key, []]), - ); + const buckets = new Map(keys.map((key) => [key, []])); for (const pullRequest of pullRequests) { buckets.get(sectionOf(pullRequest))?.push(pullRequest); diff --git a/backend/src/inbox/warm/inbox-warmer.service.ts b/backend/src/inbox/warm/inbox-warmer.service.ts index 994e856..3d27b92 100644 --- a/backend/src/inbox/warm/inbox-warmer.service.ts +++ b/backend/src/inbox/warm/inbox-warmer.service.ts @@ -1,5 +1,6 @@ import { Injectable, Logger, OnApplicationBootstrap, OnModuleDestroy } from '@nestjs/common'; import { MetricsService } from '../../analytics/metrics.service'; +import { pool } from '../../shared/async/pool'; import { getEnvConfig } from '../../shared/configs/env-configs'; import { InboxWarmTarget, UserReadService } from '../../user/read/user-read.service'; import { buildFiltersOf } from '../core/entities/inbox-filters.interface'; @@ -198,25 +199,6 @@ export class InboxWarmerService implements OnApplicationBootstrap, OnModuleDestr } } -/** - * Runs `work` over `items`, at most `limit` at a time. - * - * Workers pulling from a shared cursor rather than fixed slices, so one slow user delays the - * next item and not a whole quarter of the list. Never rejects: `warm` handles its own - * failures, and a pool that threw would take the sweep down with it. - */ -async function pool(items: T[], limit: number, work: (item: T) => Promise): Promise { - let next = 0; - - const workers = Array.from({ length: Math.min(limit, items.length) }, async () => { - for (let index = next++; index < items.length; index = next++) { - await work(items[index]); - } - }); - - await Promise.all(workers); -} - function describe(error: unknown): string { return error instanceof Error ? error.message : String(error); } diff --git a/backend/src/notifications/core/poke-settings.ts b/backend/src/notifications/core/poke-settings.ts index a257a1f..779e464 100644 --- a/backend/src/notifications/core/poke-settings.ts +++ b/backend/src/notifications/core/poke-settings.ts @@ -53,14 +53,42 @@ export function isReviewRequestResolution(value: unknown): value is ReviewReques export interface PokeSettings { mutedTypes: NotificationType[]; reviewRequestResolution: ReviewRequestResolution; + digestEnabled: boolean; + /** Nought to twenty-three, read in the timezone on the user row. */ + digestHour: number; } -/** Opting in is already an explicit act; the useful default afterwards is everything. */ +/** + * Opting in is already an explicit act; the useful default afterwards is everything. + * + * Except the digest, which is a message on a schedule rather than an answer to a webhook, and so + * is asked for rather than assumed. + */ export const DEFAULT_POKE_SETTINGS: PokeSettings = { mutedTypes: [], reviewRequestResolution: 'any_review', + digestEnabled: false, + digestHour: 9, }; +export function isDigestHour(value: unknown): value is number { + return typeof value === 'number' && Number.isInteger(value) && value >= 0 && value <= 23; +} + +/** + * A save, which is the whole set for the mutes and only what it mentions for the digest. + * + * Absence means different things: a type left out of `mutedTypes` is how unmuting is spelled, + * while a body without the digest fields is a client that predates them, and a stale tab must + * not be able to undo a schedule it has never heard of. + */ +export interface PokeSettingsUpdate { + mutedTypes: NotificationType[]; + reviewRequestResolution: ReviewRequestResolution; + digestEnabled?: boolean; + digestHour?: number; +} + /** * The settings as they sit on the user row: absent for anybody who has never moved a switch, * and plain strings rather than members of the enum. @@ -72,6 +100,8 @@ export const DEFAULT_POKE_SETTINGS: PokeSettings = { export interface PokeStoredSettings { mutedTypes?: string[]; reviewRequestResolution?: string; + digestEnabled?: boolean; + digestHour?: number; } /** @@ -96,6 +126,10 @@ export function normalizePokeSettings(stored: PokeStoredSettings | null | undefi reviewRequestResolution: isReviewRequestResolution(resolution) ? resolution : DEFAULT_POKE_SETTINGS.reviewRequestResolution, + digestEnabled: stored?.digestEnabled === true, + digestHour: isDigestHour(stored?.digestHour) + ? stored.digestHour + : DEFAULT_POKE_SETTINGS.digestHour, }; } diff --git a/backend/src/notifications/delivery/slack-message.ts b/backend/src/notifications/delivery/slack-message.ts index fcc0bf7..4bafdf1 100644 --- a/backend/src/notifications/delivery/slack-message.ts +++ b/backend/src/notifications/delivery/slack-message.ts @@ -409,6 +409,137 @@ export function buildTestMessage(githubLogin?: string): SlackMessage { }; } +export interface DigestPullRequest { + number: number; + title: string; + url: string; + repositoryFullName: string; + authorLogin: string; + /** ISO 8601, as GitHub gave it. When it was opened. */ + createdAt: string; + changedFiles: number; +} + +/** Past twenty, a count of what is left is more use than more rows. */ +const MAX_DIGEST_ROWS = 20; + +/** Slack rejects a section over 3,000 characters. */ +const MAX_SECTION_CHARS = 2800; + +const MAX_DIGEST_TITLE_CHARS = 80; + +/** + * The day's list of what is waiting on somebody, oldest first. + * + * No avatars: Slack fetches every image while posting and rejects the whole message if one + * fails, and a digest would carry one per row. + */ +export function buildDigestMessage(pullRequests: DigestPullRequest[], now: Date): SlackMessage { + const shown = pullRequests.slice(0, MAX_DIGEST_ROWS); + const hidden = pullRequests.length - shown.length; + const heading = + pullRequests.length === 1 + ? '*1 pull request is waiting on your review.*' + : `*${pullRequests.length} pull requests are waiting on your review.*`; + + const blocks: unknown[] = [{ type: 'section', text: { type: 'mrkdwn', text: heading } }]; + + for (const chunk of chunkLines(shown.map((pullRequest) => digestLine(pullRequest, now)))) { + blocks.push({ type: 'section', text: { type: 'mrkdwn', text: chunk } }); + } + + if (hidden > 0) { + blocks.push({ + type: 'context', + elements: [ + { + type: 'mrkdwn', + text: `and ${hidden} more waiting on you.`, + }, + ], + }); + } + + return { text: digestFallback(pullRequests), blocks }; +} + +/** All somebody sees in the notification preview, so it has to carry the point alone. */ +function digestFallback(pullRequests: DigestPullRequest[]): string { + const [first] = pullRequests; + + if (pullRequests.length === 1) { + return `#${first.number} ${first.title} is waiting on your review.`; + } + + return `${pullRequests.length} pull requests are waiting on your review.`; +} + +function digestLine(pullRequest: DigestPullRequest, now: Date): string { + const title = clamp(escape(pullRequest.title) || 'Untitled', MAX_DIGEST_TITLE_CHARS); + const facts = [ + age(pullRequest.createdAt, now), + files(pullRequest.changedFiles), + `\`${escape(pullRequest.repositoryFullName)}\``, + handleLink(pullRequest.authorLogin), + ]; + + return `• ${link(pullRequest.url, `#${pullRequest.number} ${title}`)} · ${facts.join(' · ')}`; +} + +/** The largest unit that still fits, rounded down, so nothing reads as older than it is. */ +function age(createdAt: string, now: Date): string { + const opened = Date.parse(createdAt); + + if (Number.isNaN(opened)) { + return 'age unknown'; + } + + const hours = Math.floor((now.getTime() - opened) / (60 * 60_000)); + + if (hours < 1) { + return 'just opened'; + } + + if (hours < 24) { + return hours === 1 ? '1 hour old' : `${hours} hours old`; + } + + const days = Math.floor(hours / 24); + + return days === 1 ? '1 day old' : `${days} days old`; +} + +function files(changed: number): string { + return changed === 1 ? '1 file' : `${changed} files`; +} + +function chunkLines(lines: string[]): string[] { + const chunks: string[] = []; + let current = ''; + + for (const line of lines) { + const candidate = current ? `${current}\n${line}` : line; + + if (candidate.length > MAX_SECTION_CHARS && current) { + chunks.push(current); + current = line; + continue; + } + + current = candidate; + } + + if (current) { + chunks.push(current); + } + + return chunks; +} + +function clamp(text: string, limit: number): string { + return text.length <= limit ? text : `${text.slice(0, limit - 1).trimEnd()}…`; +} + /** * What the link says: the title, then the number people actually use to refer to it. The * number alone is unreadable and the title alone is unsearchable. diff --git a/backend/src/notifications/delivery/slack-notification-delivery.service.ts b/backend/src/notifications/delivery/slack-notification-delivery.service.ts index 6264b6b..3d53cc7 100644 --- a/backend/src/notifications/delivery/slack-notification-delivery.service.ts +++ b/backend/src/notifications/delivery/slack-notification-delivery.service.ts @@ -19,7 +19,13 @@ import { UserNormalized } from '../../user/core/entities/user.interface'; import { GithubNotificationNormalized } from '../core/entities/github-notification.interface'; import { NotificationType } from '../core/entities/notification-type.enum'; import { PokeMessageWriteService } from '../messages/write/poke-message-write.service'; -import { buildPokeMessage, buildTestMessage, buildWelcomeMessage } from './slack-message'; +import { + buildDigestMessage, + buildPokeMessage, + buildTestMessage, + buildWelcomeMessage, + DigestPullRequest, +} from './slack-message'; /** * Why a poke did not reach Slack. Only `sent` and `failed` are unusual; the rest are ordinary @@ -191,6 +197,23 @@ export class SlackNotificationDeliveryService { return outcome; } + /** + * The day's list, from the sweep rather than anything that just happened. Not remembered the + * way a poke is: a digest is a list, with nothing in it to strike through later. + */ + public async deliverDigest( + userId: string, + pullRequests: DigestPullRequest[], + now: Date, + ): Promise { + const { outcome } = await this.send(userId, buildDigestMessage(pullRequests, now), { + trigger: 'digest', + pokeType: 'digest', + }); + + return outcome; + } + private async send( userId: string, message: SlackMessage, diff --git a/backend/src/notifications/digest/digest.module.ts b/backend/src/notifications/digest/digest.module.ts new file mode 100644 index 0000000..853661c --- /dev/null +++ b/backend/src/notifications/digest/digest.module.ts @@ -0,0 +1,19 @@ +import { Module } from '@nestjs/common'; +import { InboxModule } from '../../inbox/inbox.module'; +import { UserReadModule } from '../../user/read/user-read.module'; +import { UserWriteModule } from '../../user/write/user-write.module'; +import { NotificationsCoreModule } from '../core/notifications-core.module'; +import { DigestService } from './digest.service'; + +/** + * The timer that sends the daily digest. + * + * Above the modules it uses, like InboxWarmModule: neither the inbox that builds the list nor + * the delivery that sends it should know about a schedule. + */ +@Module({ + imports: [InboxModule, UserReadModule, UserWriteModule, NotificationsCoreModule], + providers: [DigestService], + exports: [DigestService], +}) +export class DigestModule {} diff --git a/backend/src/notifications/digest/digest.service.ts b/backend/src/notifications/digest/digest.service.ts new file mode 100644 index 0000000..325f2ee --- /dev/null +++ b/backend/src/notifications/digest/digest.service.ts @@ -0,0 +1,180 @@ +import { Injectable, Logger, OnApplicationBootstrap, OnModuleDestroy } from '@nestjs/common'; +import { MetricsService } from '../../analytics/metrics.service'; +import { buildFiltersOf } from '../../inbox/core/entities/inbox-filters.interface'; +import { InboxPullRequest } from '../../inbox/core/entities/inbox.interface'; +import { groupWaitingOnYou } from '../../inbox/inbox-classifier'; +import { InboxRefreshService } from '../../inbox/inbox-refresh.service'; +import { pool } from '../../shared/async/pool'; +import { getEnvConfig } from '../../shared/configs/env-configs'; +import { localMomentIn } from '../../shared/time/local-day'; +import { DigestTarget, UserReadService } from '../../user/read/user-read.service'; +import { UserWriteService } from '../../user/write/user-write.service'; +import { SlackNotificationDeliveryService } from '../delivery/slack-notification-delivery.service'; +import { DigestPullRequest } from '../delivery/slack-message'; + +/** Without it, a deploy during somebody's digest hour holds their digest until the next sweep. */ +const FIRST_SWEEP_DELAY_MS = 20_000; + +/** As many at once as the warmer allows itself, and for the same reason: see InboxWarmerService. */ +const CONCURRENCY = 4; + +/** + * The daily list of what is still waiting on somebody. + * + * The warmer only reads. This sends Slack messages, which cannot be taken back, so a day is + * claimed and only the pass that wins it posts. See UserWriteService.claimDigest. + */ +@Injectable() +export class DigestService implements OnApplicationBootstrap, OnModuleDestroy { + private readonly logger = new Logger(DigestService.name); + private firstSweep?: NodeJS.Timeout; + private timer?: NodeJS.Timeout; + private sweeping = false; + + constructor( + private readonly userReadService: UserReadService, + private readonly userWriteService: UserWriteService, + private readonly inboxRefreshService: InboxRefreshService, + private readonly deliveryService: SlackNotificationDeliveryService, + private readonly metrics: MetricsService, + ) {} + + public onApplicationBootstrap(): void { + const intervalMs = getEnvConfig().notifications.digestSweepIntervalMs; + + if (intervalMs <= 0) { + this.logger.log('The digest is off (DIGEST_SWEEP_INTERVAL_MS is 0)'); + + return; + } + + this.firstSweep = setTimeout(() => void this.sweep(), FIRST_SWEEP_DELAY_MS); + this.firstSweep.unref?.(); + + this.timer = setInterval(() => void this.sweep(), intervalMs); + this.timer.unref?.(); + + this.logger.log(`Digest sweep every ${Math.round(intervalMs / 60_000)}m`); + } + + public onModuleDestroy(): void { + clearTimeout(this.firstSweep); + clearInterval(this.timer); + } + + /** `now` is passed so a spec can pin the clock and test two timezones at one instant. */ + public async sweep(now: Date = new Date()): Promise { + if (this.sweeping) { + this.metrics.count('proke.digest.sweeps', { outcome: 'overlapped' }); + this.logger.warn('Skipping a digest sweep: the previous one is still running'); + + return; + } + + this.sweeping = true; + const startedAt = Date.now(); + + try { + const targets = await this.userReadService.readDigestTargets(); + + await pool( + targets.filter((target) => isDue(target, now)), + CONCURRENCY, + (target) => this.send(target, now), + ); + + this.metrics.count('proke.digest.sweeps', { outcome: 'completed' }); + } catch (error) { + this.metrics.count('proke.digest.sweeps', { outcome: 'failed' }); + this.logger.error(`Digest sweep failed: ${describe(error)}`); + } finally { + this.sweeping = false; + this.metrics.duration('proke.digest.duration', Date.now() - startedAt, {}); + } + } + + private async send(target: DigestTarget, now: Date): Promise { + try { + const result = await this.inboxRefreshService.refresh( + target.userId, + buildFiltersOf(target.settings), + ); + + // Unclaimed, so a GitHub blip at nine costs one sweep rather than the whole day. + if (!result.ok) { + this.metrics.count('proke.digest.sent', { + outcome: result.reason === 'no-token' ? 'no_token' : 'github_unavailable', + }); + + return; + } + + // Through the grouping, not the stored rows: ignoredAuthors is applied on the way out. + const waiting = groupWaitingOnYou(result.snapshot.waitingOnYou, target.settings) + .flatMap((section) => section.pullRequests) + .sort(byOldestFirst); + + const day = localMomentIn(target.timezone, now).day; + + if (!(await this.userWriteService.claimDigest(target.userId, day))) { + this.metrics.count('proke.digest.sent', { outcome: 'claimed_already' }); + + return; + } + + // Below the claim, so a quiet day is spent rather than rebuilt every quarter of an hour. + if (waiting.length === 0) { + this.metrics.count('proke.digest.sent', { outcome: 'empty' }); + + return; + } + + const outcome = await this.deliveryService.deliverDigest( + target.userId, + waiting.map(toDigestPullRequest), + now, + ); + + this.metrics.count('proke.digest.sent', { + outcome: outcome === 'sent' ? 'sent' : outcome === 'failed' ? 'failed' : 'undeliverable', + }); + } catch (error) { + this.metrics.count('proke.digest.sent', { outcome: 'failed' }); + this.logger.error(`Failed to send the digest of user ${target.userId}: ${describe(error)}`); + } + } +} + +/** `sentOn` is checked here, not left to the claim, so the rest of the day costs no writes. */ +function isDue(target: DigestTarget, now: Date): boolean { + const moment = localMomentIn(target.timezone, now); + + return moment.hour >= target.hour && target.sentOn !== moment.day; +} + +function byOldestFirst(left: InboxPullRequest, right: InboxPullRequest): number { + return openedMs(left) - openedMs(right); +} + +/** Zero rather than NaN, which would leave the whole list unordered. Same as `updatedMs`. */ +function openedMs(pullRequest: InboxPullRequest): number { + const parsed = Date.parse(pullRequest.createdAt || ''); + + return Number.isNaN(parsed) ? 0 : parsed; +} + +function toDigestPullRequest(pullRequest: InboxPullRequest): DigestPullRequest { + return { + number: pullRequest.number, + title: pullRequest.title, + url: pullRequest.url, + repositoryFullName: pullRequest.repositoryFullName, + authorLogin: pullRequest.author.login, + createdAt: pullRequest.createdAt, + changedFiles: pullRequest.changedFiles, + }; +} + +function describe(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} diff --git a/backend/src/notifications/dto/poke-settings.response.ts b/backend/src/notifications/dto/poke-settings.response.ts index 3755adf..64a4d3f 100644 --- a/backend/src/notifications/dto/poke-settings.response.ts +++ b/backend/src/notifications/dto/poke-settings.response.ts @@ -36,4 +36,23 @@ export class PokeSettingsResponse implements PokeSettings { 'An account that has never touched the settings answers `any_review`.', }) reviewRequestResolution: ReviewRequestResolution; + + @ApiProperty({ + description: + 'Whether this user gets a daily digest of the pull requests still waiting on their ' + + 'review. Off for an account that has never touched the settings: unlike every other ' + + 'kind here it is a message on a schedule rather than an answer to a webhook, so it is ' + + 'asked for rather than assumed.', + }) + digestEnabled: boolean; + + @ApiProperty({ + minimum: 0, + maximum: 23, + description: + "The hour the digest is sent, read in this user's own timezone. Nine for an account that " + + 'has never touched the settings. A digest is sent at most once a local day, so changing ' + + "this after today's has gone takes effect tomorrow.", + }) + digestHour: number; } diff --git a/backend/src/notifications/dto/update-poke-settings.body.ts b/backend/src/notifications/dto/update-poke-settings.body.ts index 094880f..00b493c 100644 --- a/backend/src/notifications/dto/update-poke-settings.body.ts +++ b/backend/src/notifications/dto/update-poke-settings.body.ts @@ -1,5 +1,16 @@ import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; -import { ArrayMaxSize, IsArray, IsEnum, IsIn, IsOptional } from 'class-validator'; +import { + ArrayMaxSize, + IsArray, + IsBoolean, + IsEnum, + IsIn, + IsInt, + IsOptional, + IsTimeZone, + Max, + Min, +} from 'class-validator'; import { ALL_NOTIFICATION_TYPES, NotificationType } from '../core/entities/notification-type.enum'; import { REVIEW_REQUEST_RESOLUTIONS, ReviewRequestResolution } from '../core/poke-settings'; @@ -29,4 +40,26 @@ export class UpdatePokeSettingsBody { @IsOptional() @IsIn(REVIEW_REQUEST_RESOLUTIONS) reviewRequestResolution?: ReviewRequestResolution; + + @ApiPropertyOptional() + @IsOptional() + @IsBoolean() + digestEnabled?: boolean; + + @ApiPropertyOptional({ minimum: 0, maximum: 23 }) + @IsOptional() + @IsInt() + @Min(0) + @Max(23) + digestHour?: number; + + /** + * The reader's IANA zone, which only the browser knows, sent with every save so somebody who + * moves is followed. Rejected rather than dropped when unknown: a digest stored against a zone + * nothing can read is one that silently never arrives. + */ + @ApiPropertyOptional() + @IsOptional() + @IsTimeZone() + timezone?: string; } diff --git a/backend/src/notifications/settings/poke-settings.controller.ts b/backend/src/notifications/settings/poke-settings.controller.ts index 5c58c20..ff68f67 100644 --- a/backend/src/notifications/settings/poke-settings.controller.ts +++ b/backend/src/notifications/settings/poke-settings.controller.ts @@ -38,11 +38,18 @@ export class PokeSettingsController { @CurrentUserId() userId: string, @Body() body: UpdatePokeSettingsBody, ): Promise { - const settings = await this.userWriteService.updatePokeSettings(userId, { - mutedTypes: body.mutedTypes, - reviewRequestResolution: - body.reviewRequestResolution ?? DEFAULT_POKE_SETTINGS.reviewRequestResolution, - }); + const settings = await this.userWriteService.updatePokeSettings( + userId, + { + mutedTypes: body.mutedTypes, + reviewRequestResolution: + body.reviewRequestResolution ?? DEFAULT_POKE_SETTINGS.reviewRequestResolution, + // Absences passed through: a client that has not heard of the digest cannot reset it. + digestEnabled: body.digestEnabled, + digestHour: body.digestHour, + }, + body.timezone, + ); // The names go in whole, unlike the inbox's team and author lists: these are our own closed // set rather than somebody else's data, and which kinds people actually switch off is the @@ -52,6 +59,8 @@ export class PokeSettingsController { muted_types: settings.mutedTypes, muted_count: settings.mutedTypes.length, review_request_resolution: settings.reviewRequestResolution, + digest_enabled: settings.digestEnabled, + digest_hour: settings.digestHour, }); return settings; diff --git a/backend/src/shared/async/pool.ts b/backend/src/shared/async/pool.ts new file mode 100644 index 0000000..e09275c --- /dev/null +++ b/backend/src/shared/async/pool.ts @@ -0,0 +1,22 @@ +/** + * Runs `work` over `items`, at most `limit` at a time. + * + * Workers pulling from a shared cursor rather than fixed slices, so one slow item delays the next + * one and not a whole quarter of the list. Never rejects: callers handle their own failures, and + * a pool that threw would take its caller down with it. + */ +export async function pool( + items: T[], + limit: number, + work: (item: T) => Promise, +): Promise { + let next = 0; + + const workers = Array.from({ length: Math.min(limit, items.length) }, async () => { + for (let index = next++; index < items.length; index = next++) { + await work(items[index]); + } + }); + + await Promise.all(workers); +} diff --git a/backend/src/shared/configs/env-configs.ts b/backend/src/shared/configs/env-configs.ts index fb5fc8c..a1920fc 100644 --- a/backend/src/shared/configs/env-configs.ts +++ b/backend/src/shared/configs/env-configs.ts @@ -91,6 +91,12 @@ interface EnvConfig { * and once per member it picks out by name. */ reviewBatchWindowMs: number; + /** + * How often the digest sweep looks for somebody whose hour has come, and nought to turn it + * off. Not how often anybody is messaged: the claim allows one digest per local day whatever + * this is set to, so it only decides how soon after their hour they hear. + */ + digestSweepIntervalMs: number; }; } @@ -164,6 +170,9 @@ export function getEnvConfig(): EnvConfig { // Five seconds: long enough that the pieces of one review reliably meet, short enough that // a poke is still a poke. Configurable mostly so the e2e suite need not sit through it. reviewBatchWindowMs: Number(process.env.REVIEW_BATCH_WINDOW_MS ?? 5000), + // A quarter hour: the coarsest that still honours zones offset by half and three-quarter + // hours, and four passes an hour means only a long deploy costs anybody their digest. + digestSweepIntervalMs: Number(process.env.DIGEST_SWEEP_INTERVAL_MS ?? 15 * 60_000), }, }; } diff --git a/backend/src/shared/time/local-day.ts b/backend/src/shared/time/local-day.ts new file mode 100644 index 0000000..5553984 --- /dev/null +++ b/backend/src/shared/time/local-day.ts @@ -0,0 +1,63 @@ +/** + * What day and hour it is where somebody is. + * + * Through Intl rather than arithmetic on a stored offset, which is only true until the clocks + * move. + */ + +export interface LocalMoment { + /** `YYYY-MM-DD` in the zone. */ + day: string; + hour: number; +} + +/** Constructing one loads zone data, and the sweep asks for the same few zones all day. */ +const FORMATTERS = new Map(); + +export function isTimezone(value: unknown): value is string { + if (typeof value !== 'string' || value.length === 0) { + return false; + } + + try { + formatterFor(value); + + return true; + } catch { + return false; + } +} + +export function localMomentIn(timezone: string, at: Date): LocalMoment { + const parts = formatterFor(timezone).formatToParts(at); + const read = (type: Intl.DateTimeFormatPartTypes): string => + parts.find((part) => part.type === type)?.value ?? ''; + + return { + day: `${read('year')}-${read('month')}-${read('day')}`, + hour: Number(read('hour')), + }; +} + +function formatterFor(timezone: string): Intl.DateTimeFormat { + const existing = FORMATTERS.get(timezone); + + if (existing) { + return existing; + } + + // Throws RangeError on an unknown zone, which is what isTimezone reads. + const formatter = new Intl.DateTimeFormat('en-GB', { + timeZone: timezone, + year: 'numeric', + month: '2-digit', + day: '2-digit', + hour: '2-digit', + // `hourCycle` rather than `hour12: false`, which renders midnight as 24 on some runtimes. + hourCycle: 'h23', + }); + + FORMATTERS.set(timezone, formatter); + + return formatter; +} diff --git a/backend/src/user/core/entities/user.entity.ts b/backend/src/user/core/entities/user.entity.ts index 16fe75c..aa6179d 100644 --- a/backend/src/user/core/entities/user.entity.ts +++ b/backend/src/user/core/entities/user.entity.ts @@ -55,6 +55,14 @@ export class PokeSettingsEntity implements PokeStoredSettings { // existed, which reads as the default - and a value this deploy cannot spell reads as it too. @Prop() reviewRequestResolution?: string; + + @Prop() + digestEnabled?: boolean; + + // An hour rather than a time: a stored "09:00+02:00" is wrong twice a year, and again when + // they move. + @Prop() + digestHour?: number; } export const PokeSettingsSchema = SchemaFactory.createForClass(PokeSettingsEntity); @@ -132,6 +140,16 @@ export class UserEntity { @Prop({ type: Date }) inboxLastUsedAt?: Date; + // An IANA zone the browser resolved: GitHub exposes none and nothing here implies one. Only + // the digest reads it, so most rows never have one. + @Prop() + timezone?: string; + + // The last local date a digest was claimed for, `YYYY-MM-DD` in this user's own zone. A date + // rather than an instant, so the once-a-day rule survives daylight saving and half-hour zones. + @Prop() + digestSentOn?: string; + @Prop() createdAt: Date; @@ -146,3 +164,7 @@ export const UserSchema = SchemaFactory.createForClass(UserEntity); // The warmer's one query is a range over this, every five minutes, across every user there is. // Sparse because most rows never get the field, and a row without it can never match a `$gte`. UserSchema.index({ inboxLastUsedAt: 1 }, { sparse: true }); + +// The digest sweep asks for everybody who has turned it on, several times an hour. Sparse for +// the same reason: absent on most rows, and `true` is the only value worth finding. +UserSchema.index({ 'pokeSettings.digestEnabled': 1 }, { sparse: true }); diff --git a/backend/src/user/read/user-read.service.ts b/backend/src/user/read/user-read.service.ts index cbef566..5132b52 100644 --- a/backend/src/user/read/user-read.service.ts +++ b/backend/src/user/read/user-read.service.ts @@ -5,7 +5,9 @@ import { InboxFilters, normalizeInboxSettings, } from '../../inbox/core/entities/inbox-filters.interface'; +import { normalizePokeSettings } from '../../notifications/core/poke-settings'; import { TokenCipherService } from '../../shared/crypto/token-cipher.service'; +import { isTimezone } from '../../shared/time/local-day'; import { UserEntity } from '../core/entities/user.entity'; import { UserNormalized } from '../core/entities/user.interface'; import { UserSerializer } from '../core/entities/user.serializer'; @@ -16,6 +18,17 @@ export interface InboxWarmTarget { settings: InboxFilters; } +/** One person who has asked for a digest, and what it takes to decide whether one is due. */ +export interface DigestTarget { + userId: string; + timezone: string; + hour: number; + /** The local day their last digest was claimed for. The claim still decides; this saves a write. */ + sentOn?: string; + /** Their inbox settings, so the digest lists what their inbox would. */ + settings: InboxFilters; +} + @Injectable() export class UserReadService { constructor( @@ -95,6 +108,41 @@ export class UserReadService { })); } + /** + * Everybody who has turned the digest on and still holds a token. Whose hour has come is the + * sweep's question, not Mongo's - it cannot evaluate a timezone. Projected for the reason the + * warmer's targets are: normalising would decrypt every stored token to answer it. + */ + public async readDigestTargets(): Promise { + const users = await this.userModel + .find({ + 'pokeSettings.digestEnabled': true, + githubAccessToken: { $exists: true, $ne: null }, + }) + .select({ _id: 1, pokeSettings: 1, timezone: 1, digestSentOn: 1, inboxSettings: 1 }) + .lean< + Pick[] + >() + .exec(); + + return users.flatMap((user) => { + // Without a zone there is no hour to be due at. + if (!isTimezone(user.timezone)) { + return []; + } + + return [ + { + userId: user._id.toString(), + timezone: user.timezone, + hour: normalizePokeSettings(user.pokeSettings).digestHour, + sentOn: user.digestSentOn, + settings: normalizeInboxSettings(user.inboxSettings), + }, + ]; + }); + } + private normalize(user: UserEntity): UserNormalized { return UserSerializer.normalize(user, (value) => this.tokenCipher.decrypt(value)); } diff --git a/backend/src/user/write/user-write.service.ts b/backend/src/user/write/user-write.service.ts index 9268bdb..b0bb89a 100644 --- a/backend/src/user/write/user-write.service.ts +++ b/backend/src/user/write/user-write.service.ts @@ -5,8 +5,13 @@ import { InboxFilters, normalizeInboxSettings, } from '../../inbox/core/entities/inbox-filters.interface'; -import { PokeSettings, normalizePokeSettings } from '../../notifications/core/poke-settings'; +import { + PokeSettings, + PokeSettingsUpdate, + normalizePokeSettings, +} from '../../notifications/core/poke-settings'; import { TokenCipherService } from '../../shared/crypto/token-cipher.service'; +import { isTimezone, localMomentIn } from '../../shared/time/local-day'; import { UserEntity } from '../core/entities/user.entity'; import { UserNormalized } from '../core/entities/user.interface'; import { UserSerializer } from '../core/entities/user.serializer'; @@ -150,21 +155,91 @@ export class UserWriteService { * as the inbox settings above. Unmuting is spelled by sending a set without that type in it, * so a merge would make it unspellable. */ - public async updatePokeSettings(userId: string, settings: PokeSettings): Promise { - const user = await this.userModel + public async updatePokeSettings( + userId: string, + settings: PokeSettingsUpdate, + timezone?: string, + now: Date = new Date(), + ): Promise { + // Field by field, not one subdocument: a body that says nothing about the digest leaves it. + const previous = await this.userModel .findOneAndUpdate( { _id: new Types.ObjectId(userId) }, - { $set: { pokeSettings: settings } }, - { returnDocument: 'after' }, + { + $set: { + 'pokeSettings.mutedTypes': settings.mutedTypes, + 'pokeSettings.reviewRequestResolution': settings.reviewRequestResolution, + ...(settings.digestEnabled === undefined + ? {} + : { 'pokeSettings.digestEnabled': settings.digestEnabled }), + ...(settings.digestHour === undefined + ? {} + : { 'pokeSettings.digestHour': settings.digestHour }), + ...(timezone ? { timezone } : {}), + }, + }, + { returnDocument: 'before' }, ) .lean() .exec(); - if (!user) { + if (!previous) { throw new NotFoundException('User not found'); } - return normalizePokeSettings(user.pokeSettings); + const before = normalizePokeSettings(previous.pokeSettings); + const after: PokeSettings = { + mutedTypes: settings.mutedTypes, + reviewRequestResolution: settings.reviewRequestResolution, + digestEnabled: settings.digestEnabled ?? before.digestEnabled, + digestHour: settings.digestHour ?? before.digestHour, + }; + + // Also when there is no stamp at all: enabling from a client that sent no timezone claimed + // nothing, and the save that later supplies one must not fire a digest minutes afterwards. + if (after.digestEnabled && (!before.digestEnabled || !previous.digestSentOn)) { + await this.claimDigestOnEnable(userId, after.digestHour, timezone ?? previous.timezone, now); + } + + return after; + } + + /** + * Takes one digest per person per day, and answers whether this caller got it. + * + * One conditional write rather than a read and a write, so two passes - or two replicas - + * cannot both find the day unclaimed and both send. + */ + public async claimDigest(userId: string, localDay: string): Promise { + const result = await this.userModel.updateOne( + { _id: new Types.ObjectId(userId), digestSentOn: { $ne: localDay } }, + { $set: { digestSentOn: localDay } }, + ); + + return result.modifiedCount === 1; + } + + /** Spends today only where their hour has been, so turning it on at four sends nothing at four. */ + private async claimDigestOnEnable( + userId: string, + hour: number, + timezone: string | undefined, + now: Date, + ): Promise { + if (!isTimezone(timezone)) { + return; + } + + const moment = localMomentIn(timezone, now); + + if (moment.hour < hour) { + return; + } + + await this.userModel.updateOne( + { _id: new Types.ObjectId(userId) }, + { $set: { digestSentOn: moment.day } }, + ); } /** diff --git a/backend/test/notifications/digest.spec.ts b/backend/test/notifications/digest.spec.ts new file mode 100644 index 0000000..d7af55e --- /dev/null +++ b/backend/test/notifications/digest.spec.ts @@ -0,0 +1,620 @@ +import * as nock from 'nock'; +import * as request from 'supertest'; +import { DEFAULT_POKE_SETTINGS } from '../../src/notifications/core/poke-settings'; +import { createTestApp } from '../utils/bootstrap'; + +const TEAM_ID = 'T0ACME'; + +/** + * The daily digest: who it is sent to, when, and what it says. + * + * 2026-09-11T08:30:00Z is 09:30 in Lisbon, 01:30 in Los Angeles and 17:30 in Tokyo, so a user + * whose hour is nine is due in two of those three and not the third. + */ +describe('The daily digest', () => { + let bootstrap: Awaited>; + + const MORNING_IN_LISBON = new Date('2026-09-11T08:30:00Z'); + const AFTERNOON_IN_LISBON = new Date('2026-09-11T13:30:00Z'); + const NEXT_MORNING_IN_LISBON = new Date('2026-09-12T08:30:00Z'); + + beforeAll(async () => { + process.env.TOKEN_ENCRYPTION_KEY = 'test-encryption-key'; + + bootstrap = await createTestApp(); + }); + + beforeEach(async () => { + await bootstrap.methods.beforeEach(); + }); + + afterAll(async () => { + await bootstrap.methods.afterAll(); + }); + + const server = () => bootstrap.app.getHttpServer(); + const digest = () => bootstrap.services.digestService; + const auth = (token: string) => ({ Authorization: `Bearer ${token}` }); + + const pullRequest = (overrides: Record = {}) => ({ + id: `node-${Math.random()}`, + number: 1, + title: 'A change', + url: 'https://github.com/acme/api/pull/1', + isDraft: false, + updatedAt: '2026-09-11T00:00:00Z', + createdAt: '2026-09-08T08:30:00Z', + changedFiles: 4, + repository: { id: 'repo-1', nameWithOwner: 'acme/api' }, + author: { __typename: 'User', login: 'bob', avatarUrl: 'https://avatars/bob' }, + reviewThreads: { nodes: [] }, + ...overrides, + }); + + /** One GraphQL answer and one teams answer: exactly what building one inbox costs. */ + const mockOneRefresh = (waitingOnYou: any[] = [pullRequest()]) => { + nock('https://api.github.com') + .post('/graphql') + .reply(200, { + data: { + viewer: { login: 'ada' }, + yours: { nodes: [] }, + waitingOnYou: { nodes: waitingOnYou }, + }, + }); + nock('https://api.github.com').get('/user/teams').query(true).reply(200, []); + }; + + /** Registered even where nothing should be sent, so an accidental post fails an assertion. */ + const capturePost = () => { + const posts: any[] = []; + + nock('https://slack.com') + .post('/api/chat.postMessage', (body) => { + posts.push(body); + return true; + }) + .times(10) + .reply(200, { ok: true }); + + return posts; + }; + + /** Counting posts alone cannot tell a skipped sweep from one GitHub refused. */ + const untouchedGithubMocks = () => + nock.pendingMocks().some((mock) => mock.includes('api.github.com')); + + /** + * Somebody connected end to end, with the digest already on. Written onto the row rather than + * through the settings route, because enabling it there deliberately uses up the current day. + */ + const digestUser = async ( + options: { + timezone?: string; + hour?: number; + enabled?: boolean; + githubAccessToken?: string | undefined; + slack?: boolean; + sentOn?: string; + } = {}, + ) => { + const { user, token } = await bootstrap.utils.authUtils.setupUser({ + githubLogin: 'ada', + githubAccessToken: 'githubAccessToken' in options ? options.githubAccessToken : 'gho_token', + }); + + await bootstrap.models.userModel.updateOne( + { _id: user.id }, + { + $set: { + timezone: options.timezone ?? 'Europe/Lisbon', + pokeSettings: { + mutedTypes: [], + digestEnabled: options.enabled ?? true, + digestHour: options.hour ?? 9, + }, + ...(options.sentOn ? { digestSentOn: options.sentOn } : {}), + }, + }, + ); + + if (options.slack !== false) { + await connectSlack(user.id); + } + + return { user, token }; + }; + + const connectSlack = async (userId: string) => { + await bootstrap.models.slackWorkspaceModel.create({ + teamId: `${TEAM_ID}-${userId}`, + teamName: 'Acme', + botUserId: 'B0PROKE', + botToken: 'xoxb-workspace-token', + }); + await bootstrap.models.slackLinkModel.create({ + userId, + teamId: `${TEAM_ID}-${userId}`, + slackUserId: 'U0ADA', + dmChannelId: 'D0ADA', + }); + }; + + /** Not the route: whether this spends today depends on the instant, so the spec holds it. */ + const enableDigest = (userId: string, hour: number, now: Date) => + bootstrap.services.userWriteService.updatePokeSettings( + userId, + { + mutedTypes: [], + reviewRequestResolution: 'any_review', + digestEnabled: true, + digestHour: hour, + }, + 'Europe/Lisbon', + now, + ); + + const storedUser = async (userId: string) => bootstrap.models.userModel.findById(userId).lean(); + + describe('who it goes to', () => { + it('sends to somebody whose hour has come', async () => { + await digestUser(); + mockOneRefresh(); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(1); + expect(posts[0].channel).toEqual('D0ADA'); + }); + + // The same instant is 09:30 in Lisbon and 01:30 in Los Angeles. + it('leaves alone somebody for whom it is not yet that hour', async () => { + await digestUser({ timezone: 'America/Los_Angeles' }); + mockOneRefresh(); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(0); + expect(untouchedGithubMocks()).toBe(true); + }); + + it('leaves alone somebody who has not asked for one', async () => { + await digestUser({ enabled: false }); + mockOneRefresh(); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(0); + expect(untouchedGithubMocks()).toBe(true); + }); + + // The rule is whether their hour has been, not whether it is now, so a deploy across it + // costs minutes rather than the day. + it('still sends to somebody whose hour went by earlier in their day', async () => { + await digestUser({ timezone: 'Asia/Tokyo' }); + mockOneRefresh(); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(1); + }); + + it('leaves alone somebody whose GitHub authorization is gone', async () => { + await digestUser({ githubAccessToken: undefined }); + mockOneRefresh(); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(0); + expect(untouchedGithubMocks()).toBe(true); + }); + + it('survives somebody who has never connected Slack', async () => { + await digestUser({ slack: false }); + mockOneRefresh(); + + await expect(digest().sweep(MORNING_IN_LISBON)).resolves.toBeUndefined(); + }); + + // Asserts the second person's message, not only that nothing threw: carrying on is the point. + it('carries on past somebody whose inbox fails to build', async () => { + await digestUser(); + await digestUser(); + + nock('https://api.github.com').post('/graphql').reply(500, {}); + mockOneRefresh(); + const posts = capturePost(); + + await expect(digest().sweep(MORNING_IN_LISBON)).resolves.toBeUndefined(); + + expect(posts).toHaveLength(1); + }); + + // ignoredAuthors is applied when the inbox is served, so a digest reading the stored rows + // straight off the snapshot would list them anyway. + it('leaves out the authors their inbox is set to ignore', async () => { + const { token } = await digestUser(); + + await request(server()) + .put('/inbox/settings') + .send({ ignoredAuthors: ['dependabot'] }) + .set(auth(token)) + .expect(200); + + mockOneRefresh([ + pullRequest({ + number: 1, + title: 'Bump lodash', + author: { __typename: 'Bot', login: 'dependabot', avatarUrl: 'https://avatars/dep' }, + }), + pullRequest({ number: 2, title: 'A real change' }), + ]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(1); + expect(posts[0].text).toEqual('#2 A real change is waiting on your review.'); + expect(JSON.stringify(posts[0].blocks)).not.toContain('Bump lodash'); + }); + + it('says nothing at all when nothing is waiting', async () => { + await digestUser(); + mockOneRefresh([]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(0); + }); + }); + + describe('the once-a-day claim', () => { + it('sends one message however many times the sweep runs', async () => { + const { user } = await digestUser(); + mockOneRefresh(); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + await digest().sweep(MORNING_IN_LISBON); + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(1); + expect((await storedUser(user.id))?.digestSentOn).toEqual('2026-09-11'); + }); + + // Otherwise a quiet day costs an inbox rebuild on every pass until midnight. + it('spends the day even when there was nothing to send', async () => { + const { user } = await digestUser(); + mockOneRefresh([]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(0); + expect((await storedUser(user.id))?.digestSentOn).toEqual('2026-09-11'); + }); + + it('tries again later in the day when GitHub could not be reached', async () => { + const { user } = await digestUser(); + nock('https://api.github.com').post('/graphql').reply(502, {}); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(0); + expect((await storedUser(user.id))?.digestSentOn).toBeUndefined(); + + mockOneRefresh(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(1); + }); + + it('sends again the next day', async () => { + const { user } = await digestUser(); + mockOneRefresh(); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + mockOneRefresh(); + await digest().sweep(NEXT_MORNING_IN_LISBON); + + expect(posts).toHaveLength(2); + expect((await storedUser(user.id))?.digestSentOn).toEqual('2026-09-12'); + }); + }); + + describe('what it says', () => { + const lines = (posts: any[]) => + posts[0].blocks + .filter((block: any) => block.type === 'section') + .map((block: any) => block.text.text); + + it('leads with the count and lists the oldest first', async () => { + await digestUser(); + mockOneRefresh([ + pullRequest({ number: 2, title: 'Newer', createdAt: '2026-09-11T04:30:00Z' }), + pullRequest({ number: 1, title: 'Older', createdAt: '2026-09-08T08:30:00Z' }), + ]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + const [heading, list] = lines(posts); + + expect(heading).toEqual('*2 pull requests are waiting on your review.*'); + expect(list.indexOf('Older')).toBeLessThan(list.indexOf('Newer')); + }); + + it('ages each row from when the pull request was opened', async () => { + await digestUser(); + mockOneRefresh([ + pullRequest({ number: 1, createdAt: '2026-09-08T08:30:00Z' }), + pullRequest({ number: 2, createdAt: '2026-09-11T04:30:00Z' }), + pullRequest({ number: 3, createdAt: '2026-09-11T08:00:00Z' }), + ]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + const [, list] = lines(posts); + + expect(list).toContain('3 days old'); + expect(list).toContain('4 hours old'); + expect(list).toContain('just opened'); + }); + + it('carries the size, the repository and the author of each row', async () => { + await digestUser(); + mockOneRefresh([pullRequest({ number: 7, title: 'Retry the paginator', changedFiles: 1 })]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + const [, list] = lines(posts); + + expect(list).toContain('#7 Retry the paginator'); + expect(list).toContain('1 file'); + expect(list).toContain('`acme/api`'); + expect(list).toContain(''); + }); + + it('references no images at all', async () => { + await digestUser(); + mockOneRefresh([pullRequest(), pullRequest({ number: 2 })]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(JSON.stringify(posts[0].blocks)).not.toContain('avatars'); + expect(posts[0].blocks.some((block: any) => block.type === 'image')).toBe(false); + }); + + it('stops listing past twenty and says how many more there were', async () => { + await digestUser(); + mockOneRefresh( + Array.from({ length: 23 }, (_unused, index) => pullRequest({ number: index + 1 })), + ); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + const context = posts[0].blocks.find((block: any) => block.type === 'context'); + + expect(lines(posts).join('\n')).toContain('*23 pull requests are waiting on your review.*'); + expect(context.elements[0].text).toEqual('and 3 more waiting on you.'); + }); + + it('carries the whole point in the notification preview', async () => { + await digestUser(); + mockOneRefresh([pullRequest({ number: 7, title: 'Retry the paginator' })]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts[0].text).toEqual('#7 Retry the paginator is waiting on your review.'); + }); + + // Twenty rows of a real repository run to ~4,400 characters, so splitting is the normal path. + it('splits a long list into sections Slack will accept', async () => { + await digestUser(); + mockOneRefresh( + Array.from({ length: 20 }, (_unused, index) => + pullRequest({ + number: index + 1, + title: 'Retry the paginator when the upstream cursor expires mid-page', + url: `https://github.com/acme/platform-services/pull/${index + 1}`, + repository: { id: 'repo-1', nameWithOwner: 'acme/platform-services' }, + }), + ), + ); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + const [heading, ...chunks] = lines(posts); + + expect(heading).toEqual('*20 pull requests are waiting on your review.*'); + expect(chunks.length).toBeGreaterThan(1); + + for (const chunk of chunks) { + expect(chunk.length).toBeLessThan(3000); + } + + // None of them fell down the gap between two sections. + for (let number = 1; number <= 20; number += 1) { + expect(chunks.join('\n')).toContain(`#${number} Retry`); + } + }); + + it('cuts a title too long to sit on one row', async () => { + await digestUser(); + mockOneRefresh([ + pullRequest({ + number: 7, + title: + 'Rework the reconciliation job so a partial page never leaves the cursor behind, ' + + 'and backfill the ones it already did', + }), + ]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + const [, list] = lines(posts); + + expect(list).toContain('Rework the reconciliation job so a partial page'); + expect(list).toContain('…'); + expect(list).not.toContain('backfill'); + }); + + it('keeps the order when one row has no readable opening time', async () => { + await digestUser(); + // `updatedAt` sets the arrival order, and this one is where NaN takes the rest with it. + mockOneRefresh([ + pullRequest({ + number: 1, + title: 'Newer', + createdAt: '2026-09-11T04:30:00Z', + updatedAt: '2026-09-11T04:00:00Z', + }), + pullRequest({ + number: 2, + title: 'Undated', + createdAt: null, + updatedAt: '2026-09-11T03:00:00Z', + }), + pullRequest({ + number: 3, + title: 'Older', + createdAt: '2026-09-05T08:30:00Z', + updatedAt: '2026-09-11T02:00:00Z', + }), + pullRequest({ + number: 4, + title: 'Middling', + createdAt: '2026-09-08T08:30:00Z', + updatedAt: '2026-09-11T01:00:00Z', + }), + ]); + const posts = capturePost(); + + await digest().sweep(MORNING_IN_LISBON); + + const [, list] = lines(posts); + + expect(list.indexOf('Older')).toBeLessThan(list.indexOf('Middling')); + expect(list.indexOf('Middling')).toBeLessThan(list.indexOf('Newer')); + expect(list).toContain('age unknown'); + }); + }); + + describe('the settings', () => { + it('is off, at nine, for somebody who has never touched it', async () => { + const { token } = await bootstrap.utils.authUtils.setupUser(); + + const { body } = await request(server()).get('/users/me').set(auth(token)).expect(200); + + expect(body.pokeSettings.digestEnabled).toBe(DEFAULT_POKE_SETTINGS.digestEnabled); + expect(body.pokeSettings.digestHour).toBe(DEFAULT_POKE_SETTINGS.digestHour); + }); + + it('stores the hour and the zone, and hands them back with the user', async () => { + const { token, user } = await bootstrap.utils.authUtils.setupUser(); + + const updated = await request(server()) + .put('/notifications/settings') + .send({ mutedTypes: [], digestEnabled: true, digestHour: 7, timezone: 'Europe/Lisbon' }) + .set(auth(token)) + .expect(200); + + expect(updated.body.digestEnabled).toBe(true); + expect(updated.body.digestHour).toBe(7); + + const me = await request(server()).get('/users/me').set(auth(token)).expect(200); + + expect(me.body.pokeSettings.digestHour).toBe(7); + expect((await storedUser(user.id))?.timezone).toEqual('Europe/Lisbon'); + }); + + it('rejects an hour and a zone it cannot read rather than storing a default', async () => { + const { token } = await bootstrap.utils.authUtils.setupUser(); + + await request(server()) + .put('/notifications/settings') + .send({ mutedTypes: [], digestHour: 24 }) + .set(auth(token)) + .expect(400); + + await request(server()) + .put('/notifications/settings') + .send({ mutedTypes: [], timezone: 'Middle/Earth' }) + .set(auth(token)) + .expect(400); + }); + + it('starts tomorrow when it is switched on after the hour has been', async () => { + const { user } = await bootstrap.utils.authUtils.setupUser({ + githubAccessToken: 'gho_token', + }); + await connectSlack(user.id); + mockOneRefresh(); + const posts = capturePost(); + + await enableDigest(user.id, 9, MORNING_IN_LISBON); + + expect((await storedUser(user.id))?.digestSentOn).toEqual('2026-09-11'); + + await digest().sweep(MORNING_IN_LISBON); + + expect(posts).toHaveLength(0); + expect(untouchedGithubMocks()).toBe(true); + }); + + it('starts today when it is switched on before the hour has been', async () => { + const { user } = await bootstrap.utils.authUtils.setupUser({ + githubAccessToken: 'gho_token', + }); + await connectSlack(user.id); + mockOneRefresh(); + const posts = capturePost(); + + await enableDigest(user.id, 14, MORNING_IN_LISBON); + + expect((await storedUser(user.id))?.digestSentOn).toBeUndefined(); + + await digest().sweep(AFTERNOON_IN_LISBON); + + expect(posts).toHaveLength(1); + }); + + it('leaves an existing schedule alone when a client saves without it', async () => { + const { token, user } = await bootstrap.utils.authUtils.setupUser(); + + await request(server()) + .put('/notifications/settings') + .send({ mutedTypes: [], digestEnabled: true, digestHour: 7, timezone: 'Europe/Lisbon' }) + .set(auth(token)) + .expect(200); + + // A tab that predates the feature: the whole set, as it knows it. + await request(server()) + .put('/notifications/settings') + .send({ mutedTypes: [] }) + .set(auth(token)) + .expect(200); + + expect((await storedUser(user.id))?.timezone).toEqual('Europe/Lisbon'); + expect((await storedUser(user.id))?.pokeSettings?.digestHour).toBe(7); + }); + }); +}); diff --git a/backend/test/notifications/poke-settings.spec.ts b/backend/test/notifications/poke-settings.spec.ts index 9e04878..97043a5 100644 --- a/backend/test/notifications/poke-settings.spec.ts +++ b/backend/test/notifications/poke-settings.spec.ts @@ -53,6 +53,8 @@ describe('Poke settings', () => { expect(response.body.pokeSettings).toEqual({ mutedTypes: [], reviewRequestResolution: 'any_review', + digestEnabled: false, + digestHour: 9, }); }); @@ -169,6 +171,8 @@ describe('Poke settings', () => { expect(response.body).toEqual({ mutedTypes: [NotificationType.IssueComment], reviewRequestResolution: 'any_review', + digestEnabled: false, + digestHour: 9, }); }); @@ -183,6 +187,8 @@ describe('Poke settings', () => { expect(response.body).toEqual({ mutedTypes: [NotificationType.IssueMention], reviewRequestResolution: 'strict', + digestEnabled: false, + digestHour: 9, }); }); diff --git a/backend/test/utils/bootstrap.ts b/backend/test/utils/bootstrap.ts index 3a1f9fb..021adec 100644 --- a/backend/test/utils/bootstrap.ts +++ b/backend/test/utils/bootstrap.ts @@ -8,6 +8,8 @@ import { RefreshTokenEntity } from '../../src/auth/session/entities/refresh-toke import { ConnectionsModule } from '../../src/connections/connections.module'; import { InboxModule } from '../../src/inbox/inbox.module'; import { InboxWarmModule } from '../../src/inbox/warm/inbox-warm.module'; +import { DigestModule } from '../../src/notifications/digest/digest.module'; +import { DigestService } from '../../src/notifications/digest/digest.service'; import { PokeSettingsModule } from '../../src/notifications/settings/poke-settings.module'; import { InboxWarmerService } from '../../src/inbox/warm/inbox-warmer.service'; import { InstallationEntity } from '../../src/installations/core/entities/installation.entity'; @@ -41,6 +43,9 @@ import { closeInMemoryMongoServer, rootMongooseTestModule } from './mongo-in-mem */ process.env.REVIEW_BATCH_WINDOW_MS ??= '150'; +/** Off for the reason below, and more so: this timer posts to Slack rather than only reading. */ +process.env.DIGEST_SWEEP_INTERVAL_MS ??= '0'; + /** * No sweeping in the background. * @@ -84,6 +89,7 @@ export async function createTestApp() { InboxModule, InboxWarmModule, PokeSettingsModule, + DigestModule, SlackModule, GithubWebhookModule, SlackEventsModule, @@ -163,6 +169,8 @@ export async function createTestApp() { // So a spec can run one pass of the warmer on demand. The timer is off in the suite - see // INBOX_WARM_SWEEP_INTERVAL_MS above - so this is the only thing that makes it sweep. inboxWarmerService: app.get(InboxWarmerService), + // Its sweep takes the instant to run at, so a spec can test two timezones at one instant. + digestService: app.get(DigestService), userReadService: app.get(UserReadService), userWriteService: app.get(UserWriteService), inMemoryCacheService, diff --git a/frontend/src/components/dashboard/Dashboard.tsx b/frontend/src/components/dashboard/Dashboard.tsx index aeeb2cf..a8d7a39 100644 --- a/frontend/src/components/dashboard/Dashboard.tsx +++ b/frontend/src/components/dashboard/Dashboard.tsx @@ -24,10 +24,19 @@ export function Dashboard() { } = useValues(slackLogic); const { loadConnection, disconnect, sendTestPoke } = useActions(slackLogic); // No load of its own: these ride in on the profile, which authLogic has already read. - const { mutedTypes, reviewRequestResolution, notice } = - useValues(pokeSettingsLogic); - const { toggleType, setReviewRequestResolution } = - useActions(pokeSettingsLogic); + const { + mutedTypes, + reviewRequestResolution, + digestEnabled, + digestHour, + notice, + } = useValues(pokeSettingsLogic); + const { + toggleType, + setReviewRequestResolution, + setDigestEnabled, + setDigestHour, + } = useActions(pokeSettingsLogic); useEffect(() => { loadConnections(); @@ -85,9 +94,13 @@ export function Dashboard() { pokes={{ mutedTypes, reviewRequestResolution, + digestEnabled, + digestHour, notice, onToggleType: toggleType, onSetReviewRequestResolution: setReviewRequestResolution, + onSetDigestEnabled: setDigestEnabled, + onSetDigestHour: setDigestHour, }} slack={{ connection: slackConnection, diff --git a/frontend/src/components/dashboard/PokesPanel.tsx b/frontend/src/components/dashboard/PokesPanel.tsx index 76742c6..41e1a8f 100644 --- a/frontend/src/components/dashboard/PokesPanel.tsx +++ b/frontend/src/components/dashboard/PokesPanel.tsx @@ -24,6 +24,11 @@ export interface PokesPanelProps { /** When a review request is struck through once somebody else reviews. */ reviewRequestResolution: ReviewRequestResolution; onSetReviewRequestResolution: (resolution: ReviewRequestResolution) => void; + /** Whether the daily list of what is still waiting arrives, and at what hour. */ + digestEnabled: boolean; + digestHour: number; + onSetDigestEnabled: (enabled: boolean) => void; + onSetDigestHour: (hour: number) => void; /** A refused save, in words. Optional so the drafts gallery renders the panel without one. */ notice?: string | null; } @@ -66,6 +71,10 @@ export function PokesPanel({ onToggleType, reviewRequestResolution, onSetReviewRequestResolution, + digestEnabled, + digestHour, + onSetDigestEnabled, + onSetDigestHour, notice, }: PokesPanelProps) { // The row the reel is showing. Follows the pointer, and stays where it was left afterwards - @@ -149,6 +158,13 @@ export function PokesPanel({ */} + + {/* Nothing under the list unless something has actually happened - a refused save, or every kind switched off. There is no standing footnote: the rows say what they do, and a line @@ -336,6 +352,84 @@ function ResolutionRow({ ); } +const CLOCK = + "M8 0a8 8 0 1 1 0 16A8 8 0 0 1 8 0ZM1.5 8a6.5 6.5 0 1 0 13 0 6.5 6.5 0 0 0-13 0Zm7-3.25v2.992l2.028.812a.75.75 0 0 1-.557 1.392l-2.5-1A.751.751 0 0 1 7 8.25v-3.5a.75.75 0 0 1 1.5 0Z"; + +/** No detail line under any of them: "9am" is the whole of what picking it does. */ +const DIGEST_HOURS: readonly SelectOption[] = Array.from( + { length: 24 }, + (_unused, hour) => ({ + value: String(hour), + title: hourName(hour), + }) +); + +function hourName(hour: number): string { + if (hour === 0) return "12am"; + if (hour === 12) return "12pm"; + + return hour < 12 ? `${hour}am` : `${hour - 12}pm`; +} + +/** + * The digest, which is a schedule rather than a kind of poke. + * + * Under the reel rather than in the list above it: a row up there is counted in the header and + * previewed in the window, and this would be the tenth of nine kinds with no card to show. + */ +function DigestRow({ + enabled, + hour, + onSetEnabled, + onSetHour, +}: { + enabled: boolean; + hour: number; + onSetEnabled: (enabled: boolean) => void; + onSetHour: (hour: number) => void; +}) { + return ( +
+
+ + + {enabled ? ( + + ) : null} +
+
+ ); +} + /** A tick, or the ring that holds its place. The same width either way, so nothing shuffles. */ function Tick({ on }: { on: boolean }) { return ( diff --git a/frontend/src/components/ui/Select.tsx b/frontend/src/components/ui/Select.tsx index 3d14e7c..5737426 100644 --- a/frontend/src/components/ui/Select.tsx +++ b/frontend/src/components/ui/Select.tsx @@ -19,8 +19,11 @@ export interface SelectOption { value: T; /** The choice, in a word or two. What the trigger shows once it is picked. */ title: string; - /** What picking it does, in one sentence. Shown under the title in the list, never on the trigger. */ - detail: string; + /** + * What picking it does, in one sentence. Shown under the title in the list, never on the + * trigger. Optional, for lists where the title is the whole answer - an hour explains itself. + */ + detail?: string; } export interface SelectProps { @@ -269,9 +272,11 @@ export function Select({ {option.title} - - {option.detail} - + {option.detail ? ( + + {option.detail} + + ) : null} {/* The same width whether or not it is drawn, so the titles line up. */} { + // The zone rides along on every save: only the browser knows it, and moving changes it. const response = await axios.put( "/notifications/settings", - settings, + { ...settings, timezone: Intl.DateTimeFormat().resolvedOptions().timeZone }, { headers: { Authorization: `Bearer ${jwtToken}` } } ); diff --git a/frontend/src/lib/logics/pokeSettingsLogic.ts b/frontend/src/lib/logics/pokeSettingsLogic.ts index b7a4552..6df06a1 100644 --- a/frontend/src/lib/logics/pokeSettingsLogic.ts +++ b/frontend/src/lib/logics/pokeSettingsLogic.ts @@ -54,6 +54,10 @@ export const pokeSettingsLogic = kea([ setReviewRequestResolution: (resolution: ReviewRequestResolution) => ({ resolution, }), + /** Turn the daily digest on or off. Same timing as a flip. */ + setDigestEnabled: (enabled: boolean) => ({ enabled }), + /** Choose the hour it arrives, in this browser's timezone. */ + setDigestHour: (hour: number) => ({ hour }), /** The whole set as it now stands on screen, ahead of the server agreeing. */ edit: (settings: PokeSettings) => ({ settings }), dismissNotice: true, @@ -119,6 +123,8 @@ export const pokeSettingsLogic = kea([ "Couldn't save that, so it's back to how it was.", toggleType: () => null, setReviewRequestResolution: () => null, + setDigestEnabled: () => null, + setDigestHour: () => null, dismissNotice: () => null, }, ], @@ -161,6 +167,14 @@ export const pokeSettingsLogic = kea([ (settings: PokeSettings): ReviewRequestResolution => settings.reviewRequestResolution, ], + digestEnabled: [ + (s) => [s.settings], + (settings: PokeSettings): boolean => settings.digestEnabled, + ], + digestHour: [ + (s) => [s.settings], + (settings: PokeSettings): number => settings.digestHour, + ], }), listeners(({ actions, values }) => ({ @@ -186,6 +200,18 @@ export const pokeSettingsLogic = kea([ reviewRequestResolution: resolution, }; + actions.edit(next); + actions.saveSettings({ settings: next }); + }, + setDigestEnabled: ({ enabled }) => { + const next: PokeSettings = { ...values.settings, digestEnabled: enabled }; + + actions.edit(next); + actions.saveSettings({ settings: next }); + }, + setDigestHour: ({ hour }) => { + const next: PokeSettings = { ...values.settings, digestHour: hour }; + actions.edit(next); actions.saveSettings({ settings: next }); },