-
Notifications
You must be signed in to change notification settings - Fork 1
feat(core): extract feedback handlers (reply, comment, ack, close) #166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
galligan
wants to merge
1
commit into
fire-5/read-handlers
Choose a base branch
from
fire-6/feedback-handlers
base: fire-5/read-handlers
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| /** | ||
| * Handler for acknowledging feedback comments. | ||
| * | ||
| * Acknowledges one or more comments locally and optionally adds a thumbs-up | ||
| * reaction on GitHub. Supports undo via the undo flag. | ||
| */ | ||
|
|
||
| import { NotFoundError, Result } from "@outfitter/contracts"; | ||
|
|
||
| import { detectAuth } from "../auth"; | ||
| import { type AckRecord, addAcks, isAcked, removeAck } from "../ack"; | ||
| import { | ||
| batchAddReactions, | ||
| deduplicateByCommentId, | ||
| partitionResolutions, | ||
| resolveBatchIds, | ||
| } from "../batch"; | ||
| import { formatDisplayId } from "../render/ids"; | ||
| import { classifyId, generateShortId } from "../short-id"; | ||
| import type { HandlerContext } from "./types"; | ||
|
|
||
| // ============================================================================= | ||
| // Types | ||
| // ============================================================================= | ||
|
|
||
| /** Input parameters for the ack handler. */ | ||
| export interface AckInput { | ||
| /** IDs to acknowledge — short IDs, comment IDs, or PR numbers */ | ||
| ids: string[]; | ||
| /** Repository (owner/repo) */ | ||
| repo: string; | ||
| /** Add thumbs-up reaction on GitHub */ | ||
| react?: boolean | undefined; | ||
| /** Undo a previous ack */ | ||
| undo?: boolean | undefined; | ||
| } | ||
|
|
||
| /** Per-ID result for ack operations. */ | ||
| export interface AckItemResult { | ||
| /** Short display ID */ | ||
| id: string; | ||
| /** Whether this operation succeeded */ | ||
| ok: boolean; | ||
| /** Error message if not ok */ | ||
| error?: string | undefined; | ||
| } | ||
|
|
||
| /** Structured output from the ack handler. */ | ||
| export interface AckOutput { | ||
| /** Number of entries acknowledged (or un-acknowledged) */ | ||
| acked: number; | ||
| /** Number of reactions added */ | ||
| reactionsAdded: number; | ||
| /** Individual results per ID */ | ||
| results: AckItemResult[]; | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // Handler | ||
| // ============================================================================= | ||
|
|
||
| /** | ||
| * Acknowledge feedback comments, optionally adding GitHub reactions. | ||
| * | ||
| * Resolves each ID from the cache, checks ack status, and records acks locally. | ||
| * When react=true, attempts to add a thumbs-up reaction (requires auth). | ||
| * When undo=true, removes existing acks instead. | ||
| * | ||
| * @param input - Ack input including IDs, repo, and options | ||
| * @param ctx - Handler context with config, db, and logger | ||
| * @returns Result containing AckOutput on success | ||
| */ | ||
| export async function ackHandler( | ||
| input: AckInput, | ||
| ctx: HandlerContext | ||
| ): Promise<Result<AckOutput, Error>> { | ||
| if (input.ids.length === 0) { | ||
| return Result.err( | ||
| new NotFoundError({ | ||
| message: "At least one ID is required.", | ||
| resourceType: "comment", | ||
| resourceId: "", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| // Filter out PR numbers (not supported in ack) | ||
| const commentIds = input.ids.filter((id) => classifyId(id) !== "pr_number"); | ||
| if (commentIds.length === 0) { | ||
| return Result.err( | ||
| new NotFoundError({ | ||
| message: "No comment IDs provided. Use PR numbers with a dedicated bulk ack command.", | ||
| resourceType: "comment", | ||
| resourceId: input.ids[0] ?? "", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| // Resolve all comment IDs in a single batch | ||
| const resolutions = await resolveBatchIds(commentIds, input.repo); | ||
| const { comments, errors } = partitionResolutions(resolutions); | ||
| const uniqueComments = deduplicateByCommentId(comments); | ||
|
|
||
| const itemResults: AckItemResult[] = []; | ||
|
|
||
| // Collect error results | ||
| for (const e of errors) { | ||
| itemResults.push({ | ||
| id: e.id, | ||
| ok: false, | ||
| error: e.error ?? "Unknown error", | ||
| }); | ||
| } | ||
|
|
||
| if (uniqueComments.length === 0) { | ||
| return Result.ok({ | ||
| acked: 0, | ||
| reactionsAdded: 0, | ||
| results: itemResults, | ||
| }); | ||
| } | ||
|
|
||
| // Handle undo: remove acks | ||
| if (input.undo) { | ||
| let removed = 0; | ||
| for (const comment of uniqueComments) { | ||
| const count = await removeAck(comment.id, input.repo); | ||
| const shortId = comment.shortId ?? formatDisplayId(generateShortId(comment.id, input.repo)); | ||
| if (count > 0) { | ||
| removed++; | ||
| itemResults.push({ id: shortId, ok: true }); | ||
| } else { | ||
| itemResults.push({ id: shortId, ok: false, error: "Not previously acknowledged." }); | ||
| } | ||
| } | ||
| return Result.ok({ acked: removed, reactionsAdded: 0, results: itemResults }); | ||
| } | ||
|
|
||
| // Setup GitHub client for reactions if requested | ||
| let client = null; | ||
| if (input.react) { | ||
| const authResult = await detectAuth(ctx.config.github_token); | ||
| if (authResult.isOk()) { | ||
| const { GitHubClient } = await import("../github"); | ||
| client = new GitHubClient(authResult.value.token); | ||
| } else { | ||
| ctx.logger.debug("No auth for reactions; acknowledging locally only."); | ||
| } | ||
| } | ||
|
|
||
| // Check which are already acked | ||
| const ackChecks = await Promise.all( | ||
| uniqueComments.map(async (r) => { | ||
| const alreadyAcked = await isAcked(r.id, input.repo); | ||
| return { ...r, alreadyAcked }; | ||
| }) | ||
| ); | ||
|
|
||
| const toAck = ackChecks.filter((r) => !r.alreadyAcked); | ||
| const alreadyAcked = ackChecks.filter((r) => r.alreadyAcked); | ||
|
|
||
| // Add reactions in parallel for newly acked items | ||
| const reactionResults = | ||
| client && toAck.length > 0 | ||
| ? await batchAddReactions(toAck.map((r) => r.id), client) | ||
| : toAck.map((r) => ({ commentId: r.id, reactionAdded: false })); | ||
|
|
||
| const reactionMap = new Map(reactionResults.map((r) => [r.commentId, r.reactionAdded])); | ||
|
|
||
| // Build and store ack records | ||
| const ackRecords: AckRecord[] = toAck.map((r) => ({ | ||
| repo: input.repo, | ||
| pr: r.entry?.pr ?? 0, | ||
| comment_id: r.id, | ||
| acked_at: new Date().toISOString(), | ||
| ...(ctx.config.user?.github_username && { acked_by: ctx.config.user.github_username }), | ||
| reaction_added: reactionMap.get(r.id) ?? false, | ||
| })); | ||
|
|
||
| if (ackRecords.length > 0) { | ||
| await addAcks(ackRecords); | ||
| } | ||
|
|
||
| const reactionsAdded = reactionResults.filter((r) => r.reactionAdded).length; | ||
|
|
||
| // Build per-ID results | ||
| for (const r of toAck) { | ||
| const shortId = r.shortId ?? formatDisplayId(generateShortId(r.id, input.repo)); | ||
| itemResults.push({ id: shortId, ok: true }); | ||
| } | ||
| for (const r of alreadyAcked) { | ||
| const shortId = r.shortId ?? formatDisplayId(generateShortId(r.id, input.repo)); | ||
| itemResults.push({ id: shortId, ok: true }); | ||
| } | ||
|
|
||
| return Result.ok({ | ||
| acked: toAck.length, | ||
| reactionsAdded, | ||
| results: itemResults, | ||
| }); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| /** | ||
| * Handler for closing feedback: resolving review threads or closing PRs. | ||
| * | ||
| * Polymorphic: accepts a comment ID (resolves the thread) or a PR number | ||
| * (closes the PR via GitHub API). Returns structured output for formatting. | ||
| */ | ||
|
|
||
| import { AuthError, NotFoundError, Result } from "@outfitter/contracts"; | ||
|
|
||
| import { detectAuth } from "../auth"; | ||
| import { queryEntries } from "../query"; | ||
| import { formatDisplayId } from "../render/ids"; | ||
| import { | ||
| buildShortIdCache, | ||
| classifyId, | ||
| resolveShortId, | ||
| } from "../short-id"; | ||
| import type { HandlerContext } from "./types"; | ||
|
|
||
| // ============================================================================= | ||
| // Types | ||
| // ============================================================================= | ||
|
|
||
| /** Input parameters for the close handler. */ | ||
| export interface CloseInput { | ||
| /** ID to close — short ID, comment ID, or PR number */ | ||
| id: string; | ||
| /** Repository (owner/repo) */ | ||
| repo: string; | ||
| /** Comment body when closing (optional) */ | ||
| body?: string | undefined; | ||
| } | ||
|
|
||
| /** Structured output from the close handler. */ | ||
| export interface CloseOutput { | ||
| /** What was closed */ | ||
| type: "thread" | "pr"; | ||
| /** Whether it was successfully closed */ | ||
| ok: boolean; | ||
| /** URL of the closed item */ | ||
| url?: string | undefined; | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // Handler | ||
| // ============================================================================= | ||
|
|
||
| /** | ||
| * Close a review thread or PR. | ||
| * | ||
| * When the ID is a PR number, closes the PR via GitHub API. | ||
| * When the ID is a comment ID (short or full), resolves the review thread | ||
| * (or acks if it's an issue comment which cannot be resolved). | ||
| * | ||
| * @param input - Close input including ID and repo | ||
| * @param ctx - Handler context with config, db, and logger | ||
| * @returns Result containing CloseOutput on success | ||
| */ | ||
| export async function closeHandler( | ||
| input: CloseInput, | ||
| ctx: HandlerContext | ||
| ): Promise<Result<CloseOutput, Error>> { | ||
| const authResult = await detectAuth(ctx.config.github_token); | ||
| if (authResult.isErr()) { | ||
| return Result.err( | ||
| new AuthError({ message: `Authentication required to close: ${authResult.error.message}` }) | ||
| ); | ||
| } | ||
|
|
||
| const { GitHubClient } = await import("../github"); | ||
| const client = new GitHubClient(authResult.value.token); | ||
|
|
||
| const repoParts = input.repo.split("/"); | ||
| const owner = repoParts[0] ?? ""; | ||
| const name = repoParts[1] ?? ""; | ||
|
|
||
| const idType = classifyId(input.id); | ||
|
|
||
| // PR number: close the PR | ||
| if (idType === "pr_number") { | ||
| const prNum = Number.parseInt(input.id, 10); | ||
| const prIdResult = await client.fetchPullRequestId(owner, name, prNum); | ||
| if (prIdResult.isErr()) { | ||
| return Result.err( | ||
| new NotFoundError({ | ||
| message: `PR #${prNum} not found in ${input.repo}.`, | ||
| resourceType: "pull_request", | ||
| resourceId: String(prNum), | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const closeResult = await client.closePullRequest(prIdResult.value); | ||
| if (closeResult.isErr()) { | ||
| return Result.err(closeResult.error); | ||
| } | ||
|
|
||
| ctx.logger.debug("PR closed", { pr: prNum, repo: input.repo }); | ||
| return Result.ok({ type: "pr", ok: true }); | ||
| } | ||
|
|
||
| // Comment ID: resolve the review thread | ||
| const entries = await queryEntries({ filters: { repo: input.repo } }); | ||
| buildShortIdCache(entries); | ||
|
|
||
| let commentId = input.id; | ||
| if (idType === "short_id") { | ||
| const mapping = resolveShortId(input.id); | ||
| if (!mapping) { | ||
| return Result.err( | ||
| new NotFoundError({ | ||
| message: `Short ID ${formatDisplayId(input.id)} not found in cache.`, | ||
| resourceType: "comment", | ||
| resourceId: input.id, | ||
| }) | ||
| ); | ||
| } | ||
| commentId = mapping.fullId; | ||
| } | ||
|
|
||
| const entry = entries.find((e) => e.id === commentId); | ||
| if (!entry) { | ||
| return Result.err( | ||
| new NotFoundError({ | ||
| message: `Comment ${input.id} not found in cache.`, | ||
| resourceType: "comment", | ||
| resourceId: input.id, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| if (entry.subtype !== "review_comment") { | ||
| // Issue comments cannot be resolved — acknowledge with reaction instead | ||
| const reactionResult = await client.addReaction(commentId, "THUMBS_UP"); | ||
| if (reactionResult.isErr()) { | ||
| ctx.logger.debug("Reaction already exists or failed", { commentId }); | ||
| } | ||
| return Result.ok({ type: "thread", ok: true }); | ||
| } | ||
|
|
||
| // Review comment: find and resolve the thread | ||
| const threadMapResult = await client.fetchReviewThreadMap(owner, name, entry.pr); | ||
| if (threadMapResult.isErr()) { | ||
| return Result.err(threadMapResult.error); | ||
| } | ||
|
|
||
| const threadId = threadMapResult.value.get(commentId); | ||
| if (!threadId) { | ||
| return Result.err( | ||
| new NotFoundError({ | ||
| message: `No review thread found for comment ${input.id}.`, | ||
| resourceType: "thread", | ||
| resourceId: commentId, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| const resolveResult = await client.resolveReviewThread(threadId); | ||
| if (resolveResult.isErr()) { | ||
| return Result.err(resolveResult.error); | ||
| } | ||
|
|
||
| ctx.logger.debug("Thread resolved", { threadId, repo: input.repo }); | ||
| return Result.ok({ type: "thread", ok: true }); | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the issue-comment branch,
addReactionerrors are swallowed and the function always returns{ ok: true }, but there is no local ack fallback in this handler. If the reaction call fails for reasons other than “already reacted” (e.g., auth/network/API errors), the operation performs no acknowledgement while reporting success, causing callers to believe the feedback was handled when it was not.Useful? React with 👍 / 👎.