From 306c7465777cfe50c1dff34cbe99193ed5431ced Mon Sep 17 00:00:00 2001 From: gentlementlegen Date: Tue, 4 Feb 2025 20:04:40 +0900 Subject: [PATCH 1/7] chore: changed anonymous arrow function to named function --- src/comment.ts | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/comment.ts b/src/comment.ts index f082c5c..4a1f8e2 100644 --- a/src/comment.ts +++ b/src/comment.ts @@ -21,27 +21,20 @@ type WithIssueNumber = T & { issueNumber: number; }; -export type PostComment = { - ( - context: Context, - message: LogReturn | Error, - options?: CommentOptions - ): Promise | null>; - lastCommentId?: number; -}; - /** * Posts a comment on a GitHub issue if the issue exists in the context payload, embedding structured metadata to it. */ -export const postComment: PostComment = async function ( +export async function postComment( context: Context, message: LogReturn | Error, options: CommentOptions = { updateComment: true, raw: false } -) { +): Promise | null> { let issueNumber; + console.log(JSON.stringify(context.payload, null, 2)); + if ("issue" in context.payload) { issueNumber = context.payload.issue.number; } else if ("pull_request" in context.payload) { @@ -77,7 +70,7 @@ export const postComment: PostComment = async function ( context.logger.info("Cannot post comment because repository is not found in the payload.", { payload: context.payload }); } return null; -}; +} async function createStructuredMetadataWithMessage(context: Context, message: LogReturn | Error, options: CommentOptions) { let logMessage; @@ -131,3 +124,5 @@ async function createStructuredMetadataWithMessage(context: Context, message: Lo // Add carriage returns to avoid any formatting issue return `${options.raw ? logMessage?.raw : logMessage?.diff}\n\n${metadataSerialized}\n`; } + +postComment.lastCommentId = null as number | null; From 19950ff4475a1de210e36a4472e47de960e400fd Mon Sep 17 00:00:00 2001 From: gentlementlegen Date: Wed, 5 Feb 2025 01:44:05 +0900 Subject: [PATCH 2/7] fix: the comments on pull-request reviews and pull requests / issues can be posted --- src/comment.ts | 62 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/src/comment.ts b/src/comment.ts index 4a1f8e2..e18a4d8 100644 --- a/src/comment.ts +++ b/src/comment.ts @@ -21,6 +21,11 @@ type WithIssueNumber = T & { issueNumber: number; }; +type PostedGithubComment = + | RestEndpointMethodTypes["issues"]["updateComment"]["response"]["data"] + | RestEndpointMethodTypes["issues"]["createComment"]["response"]["data"] + | RestEndpointMethodTypes["pulls"]["createReplyForReviewComment"]["response"]["data"]; + /** * Posts a comment on a GitHub issue if the issue exists in the context payload, embedding structured metadata to it. */ @@ -28,17 +33,17 @@ export async function postComment( context: Context, message: LogReturn | Error, options: CommentOptions = { updateComment: true, raw: false } -): Promise | null> { +): Promise | null> { let issueNumber; - - console.log(JSON.stringify(context.payload, null, 2)); + let commentId: number | null = null; if ("issue" in context.payload) { issueNumber = context.payload.issue.number; } else if ("pull_request" in context.payload) { issueNumber = context.payload.pull_request.number; + if ("comment" in context.payload) { + commentId = context.payload.comment.id; + } } else if ("discussion" in context.payload) { issueNumber = context.payload.discussion.number; } else { @@ -48,22 +53,50 @@ export async function postComment( if ("repository" in context.payload && context.payload.repository?.owner?.login) { const body = await createStructuredMetadataWithMessage(context, message, options); - if (options.updateComment && postComment.lastCommentId) { + if (options.updateComment && postComment.lastCommentId.issueCommentId && !("pull_request" in context.payload && "comment" in context.payload)) { const commentData = await context.octokit.rest.issues.updateComment({ owner: context.payload.repository.owner.login, repo: context.payload.repository.name, - comment_id: postComment.lastCommentId, + comment_id: postComment.lastCommentId.issueCommentId, body: body, }); + return { ...commentData.data, issueNumber }; - } else { - const commentData = await context.octokit.rest.issues.createComment({ + } else if (options.updateComment && "pull_request" in context.payload && "comment" in context.payload && postComment.lastCommentId.reviewCommentId) { + const commentData = await context.octokit.rest.pulls.updateReviewComment({ + body: body, owner: context.payload.repository.owner.login, repo: context.payload.repository.name, - issue_number: issueNumber, - body: body, + comment_id: postComment.lastCommentId.reviewCommentId, }); - postComment.lastCommentId = commentData.data.id; + + return { ...commentData.data, issueNumber }; + } else { + let commentData; + if (commentId) { + commentData = await context.octokit.rest.pulls.createReplyForReviewComment({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + pull_number: issueNumber, + comment_id: commentId, + body: body, + }); + postComment.lastCommentId = { + ...postComment.lastCommentId, + reviewCommentId: commentData.data.id, + }; + } else { + commentData = await context.octokit.rest.issues.createComment({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + issue_number: issueNumber, + body: body, + }); + postComment.lastCommentId = { + ...postComment.lastCommentId, + issueCommentId: commentData.data.id, + }; + } return { ...commentData.data, issueNumber }; } } else { @@ -125,4 +158,7 @@ async function createStructuredMetadataWithMessage(context: Context, message: Lo return `${options.raw ? logMessage?.raw : logMessage?.diff}\n\n${metadataSerialized}\n`; } -postComment.lastCommentId = null as number | null; +postComment.lastCommentId = { reviewCommentId: null, issueCommentId: null } as { + reviewCommentId: number | null; + issueCommentId: number | null; +}; From f6f9dde78b90e5f7763c9ed3e24dc910581050ea Mon Sep 17 00:00:00 2001 From: gentlementlegen Date: Wed, 5 Feb 2025 02:05:19 +0900 Subject: [PATCH 3/7] chore: refactored `postComment` to lower complexity --- src/comment.ts | 243 +++++++++++++++++++++++++------------------------ 1 file changed, 124 insertions(+), 119 deletions(-) diff --git a/src/comment.ts b/src/comment.ts index e18a4d8..721f92f 100644 --- a/src/comment.ts +++ b/src/comment.ts @@ -5,6 +5,7 @@ import { PluginRuntimeInfo } from "./helpers/runtime-info"; import { sanitizeMetadata } from "./util"; const HEADER_NAME = "UbiquityOS"; +const lastCommentId = { reviewCommentId: null as number | null, issueCommentId: null as number | null }; export interface CommentOptions { /* @@ -17,148 +18,152 @@ export interface CommentOptions { updateComment?: boolean; } -type WithIssueNumber = T & { - issueNumber: number; -}; - type PostedGithubComment = | RestEndpointMethodTypes["issues"]["updateComment"]["response"]["data"] | RestEndpointMethodTypes["issues"]["createComment"]["response"]["data"] | RestEndpointMethodTypes["pulls"]["createReplyForReviewComment"]["response"]["data"]; -/** - * Posts a comment on a GitHub issue if the issue exists in the context payload, embedding structured metadata to it. - */ -export async function postComment( - context: Context, - message: LogReturn | Error, - options: CommentOptions = { updateComment: true, raw: false } -): Promise | null> { - let issueNumber; - let commentId: number | null = null; - - if ("issue" in context.payload) { - issueNumber = context.payload.issue.number; - } else if ("pull_request" in context.payload) { - issueNumber = context.payload.pull_request.number; - if ("comment" in context.payload) { - commentId = context.payload.comment.id; - } - } else if ("discussion" in context.payload) { - issueNumber = context.payload.discussion.number; - } else { - context.logger.info("Cannot post comment because issue is not found in the payload."); +type WithIssueNumber = T & { + issueNumber: number; +}; + +interface IssueContext { + issueNumber: number; + commentId?: number; + owner: string; + repo: string; +} + +function getIssueNumber(context: Context): number | undefined { + if ("issue" in context.payload) return context.payload.issue.number; + if ("pull_request" in context.payload) return context.payload.pull_request.number; + if ("discussion" in context.payload) return context.payload.discussion.number; + return undefined; +} + +function getCommentId(context: Context): number | undefined { + return "pull_request" in context.payload && "comment" in context.payload ? context.payload.comment.id : undefined; +} + +function extractIssueContext(context: Context): IssueContext | null { + if (!("repository" in context.payload) || !context.payload.repository?.owner?.login) { return null; } - if ("repository" in context.payload && context.payload.repository?.owner?.login) { - const body = await createStructuredMetadataWithMessage(context, message, options); - if (options.updateComment && postComment.lastCommentId.issueCommentId && !("pull_request" in context.payload && "comment" in context.payload)) { - const commentData = await context.octokit.rest.issues.updateComment({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - comment_id: postComment.lastCommentId.issueCommentId, - body: body, - }); - - return { ...commentData.data, issueNumber }; - } else if (options.updateComment && "pull_request" in context.payload && "comment" in context.payload && postComment.lastCommentId.reviewCommentId) { - const commentData = await context.octokit.rest.pulls.updateReviewComment({ - body: body, - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - comment_id: postComment.lastCommentId.reviewCommentId, - }); - - return { ...commentData.data, issueNumber }; - } else { - let commentData; - if (commentId) { - commentData = await context.octokit.rest.pulls.createReplyForReviewComment({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - pull_number: issueNumber, - comment_id: commentId, - body: body, - }); - postComment.lastCommentId = { - ...postComment.lastCommentId, - reviewCommentId: commentData.data.id, - }; - } else { - commentData = await context.octokit.rest.issues.createComment({ - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - issue_number: issueNumber, - body: body, - }); - postComment.lastCommentId = { - ...postComment.lastCommentId, - issueCommentId: commentData.data.id, - }; - } - return { ...commentData.data, issueNumber }; - } - } else { - context.logger.info("Cannot post comment because repository is not found in the payload.", { payload: context.payload }); - } - return null; -} + const issueNumber = getIssueNumber(context); + if (!issueNumber) return null; -async function createStructuredMetadataWithMessage(context: Context, message: LogReturn | Error, options: CommentOptions) { - let logMessage; - let callingFnName; - let instigatorName; - let metadata: Metadata; + return { + issueNumber, + commentId: getCommentId(context), + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + }; +} +async function processMessage(context: Context, message: LogReturn | Error) { if (message instanceof Error) { - metadata = { + const metadata = { message: message.message, name: message.name, stack: message.stack, }; - callingFnName = message.stack?.split("\n")[2]?.match(/at (\S+)/)?.[1] ?? "anonymous"; - logMessage = context.logger.error(message.message).logMessage; - } else if (message.metadata) { - metadata = { - message: message.metadata.message, - stack: message.metadata.stack || message.metadata.error?.stack, - caller: message.metadata.caller || message.metadata.error?.stack?.split("\n")[2]?.match(/at (\S+)/)?.[1], - }; - logMessage = message.logMessage; - callingFnName = metadata.caller; - } else { - metadata = { ...message }; + return { metadata, logMessage: context.logger.error(message.message).logMessage }; } - const jsonPretty = sanitizeMetadata(metadata); - if ("installation" in context.payload && context.payload.installation && "account" in context.payload.installation) { - instigatorName = context.payload.installation?.account?.name; - } else { - instigatorName = context.payload.sender?.login || HEADER_NAME; + const metadata = message.metadata + ? { + message: message.metadata.message, + stack: message.metadata.stack || message.metadata.error?.stack, + caller: message.metadata.caller || message.metadata.error?.stack?.split("\n")[2]?.match(/at (\S+)/)?.[1], + } + : { ...message }; + + return { metadata, logMessage: message.logMessage }; +} + +function getInstigatorName(context: Context): string { + if ( + "installation" in context.payload && + context.payload.installation && + "account" in context.payload.installation && + context.payload.installation?.account?.name + ) { + return context.payload.installation?.account?.name; } + return context.payload.sender?.login || HEADER_NAME; +} + +async function createMetadataContent(context: Context, metadata: Metadata) { + const jsonPretty = sanitizeMetadata(metadata); + const instigatorName = getInstigatorName(context); const runUrl = PluginRuntimeInfo.getInstance().runUrl; const version = await PluginRuntimeInfo.getInstance().version; + const callingFnName = metadata.caller || "anonymous"; - const ubiquityMetadataHeader = `"].join("\n"); +function formatMetadataContent(logMessage: LogReturn["logMessage"], header: string, jsonPretty: string): string { + const metadataVisible = ["```json", jsonPretty, "```"].join("\n"); + const metadataHidden = [header, jsonPretty, "-->"].join("\n"); - if (logMessage?.type === "fatal") { - // if the log message is fatal, then we want to show the metadata - metadataSerialized = [metadataSerializedVisible, metadataSerializedHidden].join("\n"); - } else { - // otherwise we want to hide it - metadataSerialized = metadataSerializedHidden; - } + return logMessage?.type === "fatal" ? [metadataVisible, metadataHidden].join("\n") : metadataHidden; +} - // Add carriage returns to avoid any formatting issue - return `${options.raw ? logMessage?.raw : logMessage?.diff}\n\n${metadataSerialized}\n`; +async function createCommentBody(context: Context, message: LogReturn | Error, options: CommentOptions): Promise { + const { metadata, logMessage } = await processMessage(context, message); + const { header, jsonPretty } = await createMetadataContent(context, metadata); + const metadataContent = formatMetadataContent(logMessage, header, jsonPretty); + + return `${options.raw ? logMessage?.raw : logMessage?.diff}\n\n${metadataContent}\n`; } -postComment.lastCommentId = { reviewCommentId: null, issueCommentId: null } as { - reviewCommentId: number | null; - issueCommentId: number | null; -}; +export async function postComment( + context: Context, + message: LogReturn | Error, + options: CommentOptions = { updateComment: true, raw: false } +): Promise | null> { + const issueContext = extractIssueContext(context); + if (!issueContext) { + context.logger.info("Cannot post comment: missing issue context in payload"); + return null; + } + + const body = await createCommentBody(context, message, options); + const { issueNumber, commentId, owner, repo } = issueContext; + + if (options.updateComment && lastCommentId.issueCommentId && !("pull_request" in context.payload && "comment" in context.payload)) { + const commentData = await context.octokit.rest.issues.updateComment({ + owner, + repo, + comment_id: lastCommentId.issueCommentId, + body, + }); + return { ...commentData.data, issueNumber }; + } + + if (commentId) { + const commentData = await context.octokit.rest.pulls.createReplyForReviewComment({ + owner, + repo, + pull_number: issueNumber, + comment_id: commentId, + body, + }); + lastCommentId.reviewCommentId = commentData.data.id; + return { ...commentData.data, issueNumber }; + } + + const commentData = await context.octokit.rest.issues.createComment({ + owner, + repo, + issue_number: issueNumber, + body, + }); + lastCommentId.issueCommentId = commentData.data.id; + return { ...commentData.data, issueNumber }; +} From 8002abb2de7b3b969adb1aa50dc99e7a5ef9f733 Mon Sep 17 00:00:00 2001 From: gentlementlegen Date: Wed, 5 Feb 2025 13:17:17 +0900 Subject: [PATCH 4/7] test: added tests for comment posting to pull-requests --- src/comment.ts | 94 ++++++++++++++++++++++++++++----------- tests/comment.test.ts | 101 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 27 deletions(-) diff --git a/src/comment.ts b/src/comment.ts index 721f92f..55b829f 100644 --- a/src/comment.ts +++ b/src/comment.ts @@ -34,6 +34,64 @@ interface IssueContext { repo: string; } +async function updateIssueComment( + context: Context, + params: { owner: string; repo: string; body: string; issueNumber: number } +): Promise> { + if (!lastCommentId.issueCommentId) { + throw context.logger.error("issueCommentId is missing"); + } + const commentData = await context.octokit.rest.issues.updateComment({ + owner: params.owner, + repo: params.repo, + comment_id: lastCommentId.issueCommentId, + body: params.body, + }); + return { ...commentData.data, issueNumber: params.issueNumber }; +} + +async function updateReviewComment( + context: Context, + params: { owner: string; repo: string; body: string; issueNumber: number } +): Promise> { + if (!lastCommentId.reviewCommentId) { + throw context.logger.error("reviewCommentId is missing"); + } + const commentData = await context.octokit.rest.pulls.updateReviewComment({ + owner: params.owner, + repo: params.repo, + comment_id: lastCommentId.reviewCommentId, + body: params.body, + }); + return { ...commentData.data, issueNumber: params.issueNumber }; +} + +async function createNewComment( + context: Context, + params: { owner: string; repo: string; body: string; issueNumber: number; commentId?: number } +): Promise> { + if (params.commentId) { + const commentData = await context.octokit.rest.pulls.createReplyForReviewComment({ + owner: params.owner, + repo: params.repo, + pull_number: params.issueNumber, + comment_id: params.commentId, + body: params.body, + }); + lastCommentId.reviewCommentId = commentData.data.id; + return { ...commentData.data, issueNumber: params.issueNumber }; + } + + const commentData = await context.octokit.rest.issues.createComment({ + owner: params.owner, + repo: params.repo, + issue_number: params.issueNumber, + body: params.body, + }); + lastCommentId.issueCommentId = commentData.data.id; + return { ...commentData.data, issueNumber: params.issueNumber }; +} + function getIssueNumber(context: Context): number | undefined { if ("issue" in context.payload) return context.payload.issue.number; if ("pull_request" in context.payload) return context.payload.pull_request.number; @@ -135,35 +193,17 @@ export async function postComment( const body = await createCommentBody(context, message, options); const { issueNumber, commentId, owner, repo } = issueContext; + const params = { owner, repo, body, issueNumber }; - if (options.updateComment && lastCommentId.issueCommentId && !("pull_request" in context.payload && "comment" in context.payload)) { - const commentData = await context.octokit.rest.issues.updateComment({ - owner, - repo, - comment_id: lastCommentId.issueCommentId, - body, - }); - return { ...commentData.data, issueNumber }; - } + if (options.updateComment) { + if (lastCommentId.issueCommentId && !("pull_request" in context.payload && "comment" in context.payload)) { + return updateIssueComment(context, params); + } - if (commentId) { - const commentData = await context.octokit.rest.pulls.createReplyForReviewComment({ - owner, - repo, - pull_number: issueNumber, - comment_id: commentId, - body, - }); - lastCommentId.reviewCommentId = commentData.data.id; - return { ...commentData.data, issueNumber }; + if (lastCommentId.reviewCommentId && "pull_request" in context.payload && "comment" in context.payload) { + return updateReviewComment(context, params); + } } - const commentData = await context.octokit.rest.issues.createComment({ - owner, - repo, - issue_number: issueNumber, - body, - }); - lastCommentId.issueCommentId = commentData.data.id; - return { ...commentData.data, issueNumber }; + return createNewComment(context, { ...params, commentId }); } diff --git a/tests/comment.test.ts b/tests/comment.test.ts index 6ebb5fd..806c603 100644 --- a/tests/comment.test.ts +++ b/tests/comment.test.ts @@ -56,4 +56,105 @@ describe("Post comment tests", () => { body: expect.anything(), }); }); + + it("Should be able to post to issues and pull-request reviews", async () => { + const logger = new Logs("debug"); + const createComment = jest.fn(() => ({ + data: { + id: 1234, + }, + })); + const updateComment = jest.fn(() => ({ + data: { + id: 1234, + }, + })); + const createReplyForReviewComment = jest.fn(() => ({ + data: { + id: 5678, + }, + })); + const updateReviewComment = jest.fn(() => ({ + data: { + id: 5678, + }, + })); + jest.unstable_mockModule("@octokit/core", () => ({ + Octokit: jest.fn(() => ({ + rest: { + issues: { + createComment, + updateComment, + }, + pulls: { + createReplyForReviewComment, + updateReviewComment, + }, + }, + })), + })); + const { Octokit } = await import("@octokit/core"); + const ctxIssue = { + payload: { + pull_request: { + number: 1, + }, + repository: { + owner: { + login: "ubiquity-os", + }, + name: "plugin-sdk", + }, + }, + logger, + octokit: new Octokit(), + } as unknown as Context; + const ctxReviewComment = { + payload: { + pull_request: { + number: 1, + }, + comment: { + id: 2, + }, + repository: { + owner: { + login: "ubiquity-os", + }, + name: "plugin-sdk", + }, + }, + logger, + octokit: new Octokit(), + } as unknown as Context; + await postComment(ctxIssue, logger.ok("test"), { updateComment: true }); + await postComment(ctxIssue, logger.ok("test 2"), { updateComment: true }); + await postComment(ctxReviewComment, logger.ok("test 3"), { updateComment: true }); + await postComment(ctxReviewComment, logger.ok("test 4"), { updateComment: true }); + expect(createComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + issue_number: 1, + body: expect.anything(), + }); + expect(updateComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + comment_id: 1234, + body: expect.anything(), + }); + expect(createReplyForReviewComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + pull_number: 1, + comment_id: 2, + body: expect.anything(), + }); + expect(updateReviewComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + comment_id: 5678, + body: expect.anything(), + }); + }); }); From a8f92f7de9c2f316376a2ca00b8bea91cdc5c4cc Mon Sep 17 00:00:00 2001 From: gentlementlegen Date: Wed, 5 Feb 2025 13:32:26 +0900 Subject: [PATCH 5/7] test: fixed pull-request tests --- tests/comment.test.ts | 104 +---------------------------------------- tests/pr.test.ts | 106 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 102 deletions(-) create mode 100644 tests/pr.test.ts diff --git a/tests/comment.test.ts b/tests/comment.test.ts index 806c603..5c5535f 100644 --- a/tests/comment.test.ts +++ b/tests/comment.test.ts @@ -15,7 +15,7 @@ describe("Post comment tests", () => { id: 1234, }, })); - jest.unstable_mockModule("@octokit/core", () => ({ + const c = jest.unstable_mockModule("@octokit/core", () => ({ Octokit: jest.fn(() => ({ rest: { issues: { @@ -55,106 +55,6 @@ describe("Post comment tests", () => { comment_id: 1234, body: expect.anything(), }); - }); - - it("Should be able to post to issues and pull-request reviews", async () => { - const logger = new Logs("debug"); - const createComment = jest.fn(() => ({ - data: { - id: 1234, - }, - })); - const updateComment = jest.fn(() => ({ - data: { - id: 1234, - }, - })); - const createReplyForReviewComment = jest.fn(() => ({ - data: { - id: 5678, - }, - })); - const updateReviewComment = jest.fn(() => ({ - data: { - id: 5678, - }, - })); - jest.unstable_mockModule("@octokit/core", () => ({ - Octokit: jest.fn(() => ({ - rest: { - issues: { - createComment, - updateComment, - }, - pulls: { - createReplyForReviewComment, - updateReviewComment, - }, - }, - })), - })); - const { Octokit } = await import("@octokit/core"); - const ctxIssue = { - payload: { - pull_request: { - number: 1, - }, - repository: { - owner: { - login: "ubiquity-os", - }, - name: "plugin-sdk", - }, - }, - logger, - octokit: new Octokit(), - } as unknown as Context; - const ctxReviewComment = { - payload: { - pull_request: { - number: 1, - }, - comment: { - id: 2, - }, - repository: { - owner: { - login: "ubiquity-os", - }, - name: "plugin-sdk", - }, - }, - logger, - octokit: new Octokit(), - } as unknown as Context; - await postComment(ctxIssue, logger.ok("test"), { updateComment: true }); - await postComment(ctxIssue, logger.ok("test 2"), { updateComment: true }); - await postComment(ctxReviewComment, logger.ok("test 3"), { updateComment: true }); - await postComment(ctxReviewComment, logger.ok("test 4"), { updateComment: true }); - expect(createComment).toHaveBeenCalledWith({ - owner: "ubiquity-os", - repo: "plugin-sdk", - issue_number: 1, - body: expect.anything(), - }); - expect(updateComment).toHaveBeenCalledWith({ - owner: "ubiquity-os", - repo: "plugin-sdk", - comment_id: 1234, - body: expect.anything(), - }); - expect(createReplyForReviewComment).toHaveBeenCalledWith({ - owner: "ubiquity-os", - repo: "plugin-sdk", - pull_number: 1, - comment_id: 2, - body: expect.anything(), - }); - expect(updateReviewComment).toHaveBeenCalledWith({ - owner: "ubiquity-os", - repo: "plugin-sdk", - comment_id: 5678, - body: expect.anything(), - }); + c.clearAllMocks(); }); }); diff --git a/tests/pr.test.ts b/tests/pr.test.ts new file mode 100644 index 0000000..06af69b --- /dev/null +++ b/tests/pr.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it, jest } from "@jest/globals"; +import { Logs } from "@ubiquity-os/ubiquity-os-logger"; +import { Context, postComment } from "../src"; + +describe("Pull-request comment tests", () => { + it("Should be able to post to issues and pull-request reviews", async () => { + const logger = new Logs("debug"); + const createComment = jest.fn(() => ({ + data: { + id: 1234, + }, + })); + const updateComment = jest.fn(() => ({ + data: { + id: 1234, + }, + })); + const createReplyForReviewComment = jest.fn(() => ({ + data: { + id: 5678, + }, + })); + const updateReviewComment = jest.fn(() => ({ + data: { + id: 5678, + }, + })); + jest.unstable_mockModule("@octokit/core", () => ({ + Octokit: jest.fn(() => ({ + rest: { + issues: { + createComment, + updateComment, + }, + pulls: { + createReplyForReviewComment, + updateReviewComment, + }, + }, + })), + })); + const { Octokit } = await import("@octokit/core"); + const ctxIssue = { + payload: { + pull_request: { + number: 1, + }, + repository: { + owner: { + login: "ubiquity-os", + }, + name: "plugin-sdk", + }, + }, + logger, + octokit: new Octokit(), + } as unknown as Context; + const ctxReviewComment = { + payload: { + pull_request: { + number: 1, + }, + comment: { + id: 2, + }, + repository: { + owner: { + login: "ubiquity-os", + }, + name: "plugin-sdk", + }, + }, + logger, + octokit: new Octokit(), + } as unknown as Context; + await postComment(ctxIssue, logger.ok("test"), { updateComment: true }); + await postComment(ctxIssue, logger.ok("test 2"), { updateComment: true }); + await postComment(ctxReviewComment, logger.ok("test 3"), { updateComment: true }); + await postComment(ctxReviewComment, logger.ok("test 4"), { updateComment: true }); + expect(createComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + issue_number: 1, + body: expect.anything(), + }); + expect(updateComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + comment_id: 1234, + body: expect.anything(), + }); + expect(createReplyForReviewComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + pull_number: 1, + comment_id: 2, + body: expect.anything(), + }); + expect(updateReviewComment).toHaveBeenCalledWith({ + owner: "ubiquity-os", + repo: "plugin-sdk", + comment_id: 5678, + body: expect.anything(), + }); + }); +}); From 7a18787a212c0f9300aa4f2dfef0843f2f150fe3 Mon Sep 17 00:00:00 2001 From: gentlementlegen Date: Thu, 6 Feb 2025 09:08:52 +0900 Subject: [PATCH 6/7] chore: added original metadata to the posted comment --- src/comment.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/comment.ts b/src/comment.ts index 55b829f..51fe056 100644 --- a/src/comment.ts +++ b/src/comment.ts @@ -131,6 +131,7 @@ async function processMessage(context: Context, message: LogReturn | Error) { const metadata = message.metadata ? { + ...message.metadata, message: message.metadata.message, stack: message.metadata.stack || message.metadata.error?.stack, caller: message.metadata.caller || message.metadata.error?.stack?.split("\n")[2]?.match(/at (\S+)/)?.[1], From da526ffb2dbb4b008af87f60e7755c4f12922af2 Mon Sep 17 00:00:00 2001 From: gentlementlegen Date: Thu, 6 Feb 2025 09:53:05 +0900 Subject: [PATCH 7/7] fix!: postComment is now wrapped inside a class to avoid instance collisions --- src/actions.ts | 5 +- src/comment.ts | 286 +++++++++++++++++++++--------------------- src/context.ts | 2 + src/index.ts | 2 +- src/server.ts | 7 +- tests/comment.test.ts | 7 +- tests/pr.test.ts | 11 +- 7 files changed, 164 insertions(+), 156 deletions(-) diff --git a/src/actions.ts b/src/actions.ts index d1b1e63..ca0dcf0 100644 --- a/src/actions.ts +++ b/src/actions.ts @@ -4,7 +4,7 @@ import { EmitterWebhookEventName as WebhookEventName } from "@octokit/webhooks"; import { Value } from "@sinclair/typebox/value"; import { LogReturn, Logs } from "@ubiquity-os/ubiquity-os-logger"; import { config } from "dotenv"; -import { postComment } from "./comment"; +import { CommentHandler } from "./comment"; import { Context } from "./context"; import { customOctokit } from "./octokit"; import { verifySignature } from "./signature"; @@ -86,6 +86,7 @@ export async function createActionsPlugin> { - if (!lastCommentId.issueCommentId) { - throw context.logger.error("issueCommentId is missing"); - } - const commentData = await context.octokit.rest.issues.updateComment({ - owner: params.owner, - repo: params.repo, - comment_id: lastCommentId.issueCommentId, - body: params.body, - }); - return { ...commentData.data, issueNumber: params.issueNumber }; -} +export class CommentHandler { + public static readonly HEADER_NAME = "UbiquityOS"; + private _lastCommentId = { reviewCommentId: null as number | null, issueCommentId: null as number | null }; -async function updateReviewComment( - context: Context, - params: { owner: string; repo: string; body: string; issueNumber: number } -): Promise> { - if (!lastCommentId.reviewCommentId) { - throw context.logger.error("reviewCommentId is missing"); + async _updateIssueComment( + context: Context, + params: { owner: string; repo: string; body: string; issueNumber: number } + ): Promise> { + if (!this._lastCommentId.issueCommentId) { + throw context.logger.error("issueCommentId is missing"); + } + const commentData = await context.octokit.rest.issues.updateComment({ + owner: params.owner, + repo: params.repo, + comment_id: this._lastCommentId.issueCommentId, + body: params.body, + }); + return { ...commentData.data, issueNumber: params.issueNumber }; } - const commentData = await context.octokit.rest.pulls.updateReviewComment({ - owner: params.owner, - repo: params.repo, - comment_id: lastCommentId.reviewCommentId, - body: params.body, - }); - return { ...commentData.data, issueNumber: params.issueNumber }; -} -async function createNewComment( - context: Context, - params: { owner: string; repo: string; body: string; issueNumber: number; commentId?: number } -): Promise> { - if (params.commentId) { - const commentData = await context.octokit.rest.pulls.createReplyForReviewComment({ + async _updateReviewComment( + context: Context, + params: { owner: string; repo: string; body: string; issueNumber: number } + ): Promise> { + if (!this._lastCommentId.reviewCommentId) { + throw context.logger.error("reviewCommentId is missing"); + } + const commentData = await context.octokit.rest.pulls.updateReviewComment({ owner: params.owner, repo: params.repo, - pull_number: params.issueNumber, - comment_id: params.commentId, + comment_id: this._lastCommentId.reviewCommentId, body: params.body, }); - lastCommentId.reviewCommentId = commentData.data.id; return { ...commentData.data, issueNumber: params.issueNumber }; } - const commentData = await context.octokit.rest.issues.createComment({ - owner: params.owner, - repo: params.repo, - issue_number: params.issueNumber, - body: params.body, - }); - lastCommentId.issueCommentId = commentData.data.id; - return { ...commentData.data, issueNumber: params.issueNumber }; -} + async _createNewComment( + context: Context, + params: { owner: string; repo: string; body: string; issueNumber: number; commentId?: number } + ): Promise> { + if (params.commentId) { + const commentData = await context.octokit.rest.pulls.createReplyForReviewComment({ + owner: params.owner, + repo: params.repo, + pull_number: params.issueNumber, + comment_id: params.commentId, + body: params.body, + }); + this._lastCommentId.reviewCommentId = commentData.data.id; + return { ...commentData.data, issueNumber: params.issueNumber }; + } -function getIssueNumber(context: Context): number | undefined { - if ("issue" in context.payload) return context.payload.issue.number; - if ("pull_request" in context.payload) return context.payload.pull_request.number; - if ("discussion" in context.payload) return context.payload.discussion.number; - return undefined; -} + const commentData = await context.octokit.rest.issues.createComment({ + owner: params.owner, + repo: params.repo, + issue_number: params.issueNumber, + body: params.body, + }); + this._lastCommentId.issueCommentId = commentData.data.id; + return { ...commentData.data, issueNumber: params.issueNumber }; + } -function getCommentId(context: Context): number | undefined { - return "pull_request" in context.payload && "comment" in context.payload ? context.payload.comment.id : undefined; -} + _getIssueNumber(context: Context): number | undefined { + if ("issue" in context.payload) return context.payload.issue.number; + if ("pull_request" in context.payload) return context.payload.pull_request.number; + if ("discussion" in context.payload) return context.payload.discussion.number; + return undefined; + } -function extractIssueContext(context: Context): IssueContext | null { - if (!("repository" in context.payload) || !context.payload.repository?.owner?.login) { - return null; + _getCommentId(context: Context): number | undefined { + return "pull_request" in context.payload && "comment" in context.payload ? context.payload.comment.id : undefined; } - const issueNumber = getIssueNumber(context); - if (!issueNumber) return null; + _extractIssueContext(context: Context): IssueContext | null { + if (!("repository" in context.payload) || !context.payload.repository?.owner?.login) { + return null; + } - return { - issueNumber, - commentId: getCommentId(context), - owner: context.payload.repository.owner.login, - repo: context.payload.repository.name, - }; -} + const issueNumber = this._getIssueNumber(context); + if (!issueNumber) return null; -async function processMessage(context: Context, message: LogReturn | Error) { - if (message instanceof Error) { - const metadata = { - message: message.message, - name: message.name, - stack: message.stack, + return { + issueNumber, + commentId: this._getCommentId(context), + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, }; - return { metadata, logMessage: context.logger.error(message.message).logMessage }; } - const metadata = message.metadata - ? { - ...message.metadata, - message: message.metadata.message, - stack: message.metadata.stack || message.metadata.error?.stack, - caller: message.metadata.caller || message.metadata.error?.stack?.split("\n")[2]?.match(/at (\S+)/)?.[1], - } - : { ...message }; + async _processMessage(context: Context, message: LogReturn | Error) { + if (message instanceof Error) { + const metadata = { + message: message.message, + name: message.name, + stack: message.stack, + }; + return { metadata, logMessage: context.logger.error(message.message).logMessage }; + } - return { metadata, logMessage: message.logMessage }; -} + const metadata = message.metadata + ? { + ...message.metadata, + message: message.metadata.message, + stack: message.metadata.stack || message.metadata.error?.stack, + caller: message.metadata.caller || message.metadata.error?.stack?.split("\n")[2]?.match(/at (\S+)/)?.[1], + } + : { ...message }; -function getInstigatorName(context: Context): string { - if ( - "installation" in context.payload && - context.payload.installation && - "account" in context.payload.installation && - context.payload.installation?.account?.name - ) { - return context.payload.installation?.account?.name; + return { metadata, logMessage: message.logMessage }; } - return context.payload.sender?.login || HEADER_NAME; -} -async function createMetadataContent(context: Context, metadata: Metadata) { - const jsonPretty = sanitizeMetadata(metadata); - const instigatorName = getInstigatorName(context); - const runUrl = PluginRuntimeInfo.getInstance().runUrl; - const version = await PluginRuntimeInfo.getInstance().version; - const callingFnName = metadata.caller || "anonymous"; - - return { - header: `"].join("\n"); + _getInstigatorName(context: Context): string { + if ( + "installation" in context.payload && + context.payload.installation && + "account" in context.payload.installation && + context.payload.installation?.account?.name + ) { + return context.payload.installation?.account?.name; + } + return context.payload.sender?.login || CommentHandler.HEADER_NAME; + } - return logMessage?.type === "fatal" ? [metadataVisible, metadataHidden].join("\n") : metadataHidden; -} + async _createMetadataContent(context: Context, metadata: Metadata) { + const jsonPretty = sanitizeMetadata(metadata); + const instigatorName = this._getInstigatorName(context); + const runUrl = PluginRuntimeInfo.getInstance().runUrl; + const version = await PluginRuntimeInfo.getInstance().version; + const callingFnName = metadata.caller || "anonymous"; -async function createCommentBody(context: Context, message: LogReturn | Error, options: CommentOptions): Promise { - const { metadata, logMessage } = await processMessage(context, message); - const { header, jsonPretty } = await createMetadataContent(context, metadata); - const metadataContent = formatMetadataContent(logMessage, header, jsonPretty); + return { + header: `"].join("\n"); -export async function postComment( - context: Context, - message: LogReturn | Error, - options: CommentOptions = { updateComment: true, raw: false } -): Promise | null> { - const issueContext = extractIssueContext(context); - if (!issueContext) { - context.logger.info("Cannot post comment: missing issue context in payload"); - return null; + return logMessage?.type === "fatal" ? [metadataVisible, metadataHidden].join("\n") : metadataHidden; } - const body = await createCommentBody(context, message, options); - const { issueNumber, commentId, owner, repo } = issueContext; - const params = { owner, repo, body, issueNumber }; + async _createCommentBody(context: Context, message: LogReturn | Error, options: CommentOptions): Promise { + const { metadata, logMessage } = await this._processMessage(context, message); + const { header, jsonPretty } = await this._createMetadataContent(context, metadata); + const metadataContent = this._formatMetadataContent(logMessage, header, jsonPretty); - if (options.updateComment) { - if (lastCommentId.issueCommentId && !("pull_request" in context.payload && "comment" in context.payload)) { - return updateIssueComment(context, params); + return `${options.raw ? logMessage?.raw : logMessage?.diff}\n\n${metadataContent}\n`; + } + + async postComment( + context: Context, + message: LogReturn | Error, + options: CommentOptions = { updateComment: true, raw: false } + ): Promise | null> { + const issueContext = this._extractIssueContext(context); + if (!issueContext) { + context.logger.info("Cannot post comment: missing issue context in payload"); + return null; } - if (lastCommentId.reviewCommentId && "pull_request" in context.payload && "comment" in context.payload) { - return updateReviewComment(context, params); + const body = await this._createCommentBody(context, message, options); + const { issueNumber, commentId, owner, repo } = issueContext; + const params = { owner, repo, body, issueNumber }; + + if (options.updateComment) { + if (this._lastCommentId.issueCommentId && !("pull_request" in context.payload && "comment" in context.payload)) { + return this._updateIssueComment(context, params); + } + + if (this._lastCommentId.reviewCommentId && "pull_request" in context.payload && "comment" in context.payload) { + return this._updateReviewComment(context, params); + } } - } - return createNewComment(context, { ...params, commentId }); + return this._createNewComment(context, { ...params, commentId }); + } } diff --git a/src/context.ts b/src/context.ts index 47b6933..f1436a0 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1,5 +1,6 @@ import { EmitterWebhookEvent as WebhookEvent, EmitterWebhookEventName as WebhookEventName } from "@octokit/webhooks"; import { Logs } from "@ubiquity-os/ubiquity-os-logger"; +import { CommentHandler } from "./comment"; import { customOctokit } from "./octokit"; export interface Context { @@ -12,4 +13,5 @@ export interface Context { + app.post("/", async function appPost(ctx) { if (ctx.req.header("content-type") !== "application/json") { throw new HTTPException(400, { message: "Content-Type must be application/json" }); } @@ -92,6 +92,7 @@ export function createPlugin { it("Should reuse a message if the reuse option is true", async () => { @@ -41,8 +41,9 @@ describe("Post comment tests", () => { logger, octokit: new Octokit(), } as unknown as Context; - await postComment(ctx, logger.ok("test"), { updateComment: true }); - await postComment(ctx, logger.ok("test 2"), { updateComment: true }); + const commentHandler = new CommentHandler(); + await commentHandler.postComment(ctx, logger.ok("test"), { updateComment: true }); + await commentHandler.postComment(ctx, logger.ok("test 2"), { updateComment: true }); expect(createComment).toHaveBeenCalledWith({ owner: "ubiquity-os", repo: "plugin-sdk", diff --git a/tests/pr.test.ts b/tests/pr.test.ts index 06af69b..e1e9aa9 100644 --- a/tests/pr.test.ts +++ b/tests/pr.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, jest } from "@jest/globals"; import { Logs } from "@ubiquity-os/ubiquity-os-logger"; -import { Context, postComment } from "../src"; +import { CommentHandler, Context } from "../src"; describe("Pull-request comment tests", () => { it("Should be able to post to issues and pull-request reviews", async () => { @@ -73,10 +73,11 @@ describe("Pull-request comment tests", () => { logger, octokit: new Octokit(), } as unknown as Context; - await postComment(ctxIssue, logger.ok("test"), { updateComment: true }); - await postComment(ctxIssue, logger.ok("test 2"), { updateComment: true }); - await postComment(ctxReviewComment, logger.ok("test 3"), { updateComment: true }); - await postComment(ctxReviewComment, logger.ok("test 4"), { updateComment: true }); + const commentHandler = new CommentHandler(); + await commentHandler.postComment(ctxIssue, logger.ok("test"), { updateComment: true }); + await commentHandler.postComment(ctxIssue, logger.ok("test 2"), { updateComment: true }); + await commentHandler.postComment(ctxReviewComment, logger.ok("test 3"), { updateComment: true }); + await commentHandler.postComment(ctxReviewComment, logger.ok("test 4"), { updateComment: true }); expect(createComment).toHaveBeenCalledWith({ owner: "ubiquity-os", repo: "plugin-sdk",