From cec1c3bf3569f4ab7d7e8c7e134b75f9898869e6 Mon Sep 17 00:00:00 2001 From: Mooncake Date: Fri, 17 Jul 2026 12:48:57 +0000 Subject: [PATCH] Fix PR comment sections colliding for multiple integrations (closes #18) When multiple kickflip PR comment integrations run in the same workflow without an explicit --action-name, they all defaulted to the 'default' section name and overwrote each other, so only the last integration's output survived in the reused comment. Key decisions: - Added PullRequestCommentComposer.ResolveSectionName which falls back to the deployment path when no explicit action name is set, so distinct flows keep distinct sections and are combined in the shared comment. - Wired the resolver into the github pull-request handler. - Root path '/' and no path still resolve to 'default'. Files changed: - src/kickflip/Services/PullRequestCommentComposer.cs - src/kickflip/Program.cs - src/kickflip.Tests/PullRequestCommentComposerTests.cs - README.md --- README.md | 2 ++ .../PullRequestCommentComposerTests.cs | 35 +++++++++++++++++++ src/kickflip/Program.cs | 3 +- .../Services/PullRequestCommentComposer.cs | 27 ++++++++++++++ 4 files changed, 66 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 2f58dff..0f5c031 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,8 @@ If you have multiple kickflip flows in the same workflow (for example deploying kickflip github pull-request --repo / --ref --token --action-name staging kickflip github pull-request --repo / --ref --token --action-name production +If you don't set `--action-name`, kickflip falls back to the `--deployment-path` to name each section. This means multiple flows deploying to different paths automatically keep their own section and no longer overwrite each other. Only flows sharing the same deployment path (and no explicit action name) will share a section. + When running inside GitHub Actions, kickflip also writes the deployment change summary to the [job summary](https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/) so it appears on the workflow run page. This happens automatically whenever the `GITHUB_STEP_SUMMARY` environment variable is present (which GitHub Actions sets for every step), in addition to posting the pull request comment. ## Github Actions diff --git a/src/kickflip.Tests/PullRequestCommentComposerTests.cs b/src/kickflip.Tests/PullRequestCommentComposerTests.cs index 234ecf7..b0d5948 100644 --- a/src/kickflip.Tests/PullRequestCommentComposerTests.cs +++ b/src/kickflip.Tests/PullRequestCommentComposerTests.cs @@ -84,6 +84,41 @@ public void Compose_IsIdempotentAcrossMultipleReuses() Assert.Equal(1, CountOccurrences(body, PullRequestCommentComposer.CommentMarker)); } + [Fact] + public void ResolveSectionName_UsesExplicitActionName_WhenProvided() + { + Assert.Equal("staging", PullRequestCommentComposer.ResolveSectionName("staging", "/some/path")); + } + + [Fact] + public void ResolveSectionName_FallsBackToDeploymentPath_WhenNoActionName() + { + Assert.Equal("/staging", PullRequestCommentComposer.ResolveSectionName(null, "/staging")); + Assert.Equal("/production", PullRequestCommentComposer.ResolveSectionName("default", "/production")); + } + + [Fact] + public void ResolveSectionName_UsesDefault_WhenNoActionNameAndRootPath() + { + Assert.Equal("default", PullRequestCommentComposer.ResolveSectionName(null, "/")); + Assert.Equal("default", PullRequestCommentComposer.ResolveSectionName("default", null)); + } + + [Fact] + public void MultipleDefaultFlows_WithDifferentDeploymentPaths_KeepBothSections() + { + // Simulates two kickflip integrations in one workflow that don't set --action-name + // but deploy to different paths. Both sections must survive. + var stagingName = PullRequestCommentComposer.ResolveSectionName(null, "/staging"); + var productionName = PullRequestCommentComposer.ResolveSectionName(null, "/production"); + + var first = PullRequestCommentComposer.Compose(null, stagingName, "staging-content"); + var second = PullRequestCommentComposer.Compose(first, productionName, "production-content"); + + Assert.Contains("staging-content", second); + Assert.Contains("production-content", second); + } + private static int CountOccurrences(string haystack, string needle) { var count = 0; diff --git a/src/kickflip/Program.cs b/src/kickflip/Program.cs index 129f3bb..bc8c861 100644 --- a/src/kickflip/Program.cs +++ b/src/kickflip/Program.cs @@ -221,7 +221,8 @@ private static async Task HandleGithubPullRequest(string localPath, FindMod Console.WriteLine(outputService.GetChangesConsole(changes)); var sectionContent = outputService.GetChangesMarkdown(changes); - var result = await gitHubService.PullRequestCommentChanges(repository, pullRequestReference, sectionContent, actionName); + var sectionName = PullRequestCommentComposer.ResolveSectionName(actionName, deploymentPath); + var result = await gitHubService.PullRequestCommentChanges(repository, pullRequestReference, sectionContent, sectionName); if (!result) { diff --git a/src/kickflip/Services/PullRequestCommentComposer.cs b/src/kickflip/Services/PullRequestCommentComposer.cs index 3d427bd..0f71980 100644 --- a/src/kickflip/Services/PullRequestCommentComposer.cs +++ b/src/kickflip/Services/PullRequestCommentComposer.cs @@ -28,6 +28,33 @@ public static bool IsKickflipComment(string? body) return body != null && body.Contains(CommentMarker, StringComparison.Ordinal); } + /// + /// Resolves the section name that a kickflip flow owns within the shared + /// comment. When an explicit is provided it is + /// used verbatim. Otherwise the is used so + /// that multiple flows in the same workflow (for example deploying to + /// different paths) keep their own section instead of all colliding on the + /// default name and overwriting each other. + /// + public static string ResolveSectionName(string? actionName, string? deploymentPath) + { + if (!string.IsNullOrWhiteSpace(actionName) && actionName.Trim() != DefaultActionName) + { + return actionName.Trim(); + } + + if (!string.IsNullOrWhiteSpace(deploymentPath)) + { + var normalized = deploymentPath.Trim(); + if (normalized != "/") + { + return normalized; + } + } + + return DefaultActionName; + } + /// /// Produces the full comment body for a kickflip comment, upserting the /// section owned by with .