diff --git a/packages/action/__tests__/compare.test.ts b/packages/action/__tests__/compare.test.ts index 55fc7212e..035af29be 100644 --- a/packages/action/__tests__/compare.test.ts +++ b/packages/action/__tests__/compare.test.ts @@ -330,6 +330,189 @@ describe("compareWithBase", () => { }); }); + test("compares against output file path when provided", async () => { + const explicitResultWithOutput: MinifyResult = { + files: [ + { + file: "src/app.js", + outputFile: "dist/app.min.js", + originalSize: 10000, + minifiedSize: 3000, + reduction: 70, + timeMs: 50, + }, + ], + compressor: "terser", + totalOriginalSize: 10000, + totalMinifiedSize: 3000, + totalReduction: 70, + totalTimeMs: 50, + }; + + (context as { payload: Record }).payload = { + pull_request: { base: { ref: "main" } }, + }; + + const mockGetContent = vi.fn().mockResolvedValue({ + data: { type: "file", size: 3500 }, + }); + + vi.mocked(getOctokit).mockReturnValue({ + rest: { + repos: { getContent: mockGetContent }, + }, + } as unknown as ReturnType); + + const result = await compareWithBase(explicitResultWithOutput, "token"); + + expect(result[0]?.file).toBe("src/app.js"); + expect(mockGetContent).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + path: "dist/app.min.js", + ref: "main", + }); + }); + + test("normalizes windows separators in output compare path", async () => { + const explicitResultWithWindowsPath: MinifyResult = { + files: [ + { + file: "src/app.js", + outputFile: "dist\\app.min.js", + originalSize: 10000, + minifiedSize: 3000, + reduction: 70, + timeMs: 50, + }, + ], + compressor: "terser", + totalOriginalSize: 10000, + totalMinifiedSize: 3000, + totalReduction: 70, + totalTimeMs: 50, + }; + + (context as { payload: Record }).payload = { + pull_request: { base: { ref: "main" } }, + }; + + const mockGetContent = vi.fn().mockResolvedValue({ + data: { type: "file", size: 3500 }, + }); + + vi.mocked(getOctokit).mockReturnValue({ + rest: { + repos: { getContent: mockGetContent }, + }, + } as unknown as ReturnType); + + await compareWithBase(explicitResultWithWindowsPath, "token"); + + expect(mockGetContent).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + path: "dist/app.min.js", + ref: "main", + }); + }); + + test("skips unsafe output compare path and falls back to source path", async () => { + const explicitResultWithUnsafeOutput: MinifyResult = { + files: [ + { + file: "src/app.js", + outputFile: "../secrets.env", + originalSize: 10000, + minifiedSize: 3000, + reduction: 70, + timeMs: 50, + }, + ], + compressor: "terser", + totalOriginalSize: 10000, + totalMinifiedSize: 3000, + totalReduction: 70, + totalTimeMs: 50, + }; + + (context as { payload: Record }).payload = { + pull_request: { base: { ref: "main" } }, + }; + + const mockGetContent = vi.fn().mockResolvedValue({ + data: { type: "file", size: 3500 }, + }); + + vi.mocked(getOctokit).mockReturnValue({ + rest: { + repos: { getContent: mockGetContent }, + }, + } as unknown as ReturnType); + + await compareWithBase(explicitResultWithUnsafeOutput, "token"); + + expect(mockGetContent).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + path: "src/app.js", + ref: "main", + }); + expect(warning).toHaveBeenCalledWith( + "Skipping unsafe base-compare path: ../secrets.env" + ); + }); + + test("marks file as new when both output and source compare paths are unsafe", async () => { + const resultWithUnsafePaths: MinifyResult = { + files: [ + { + file: "..", + outputFile: "../secrets.env", + originalSize: 10000, + minifiedSize: 3000, + reduction: 70, + timeMs: 50, + }, + ], + compressor: "terser", + totalOriginalSize: 10000, + totalMinifiedSize: 3000, + totalReduction: 70, + totalTimeMs: 50, + }; + + (context as { payload: Record }).payload = { + pull_request: { base: { ref: "main" } }, + }; + + const mockGetContent = vi.fn(); + vi.mocked(getOctokit).mockReturnValue({ + rest: { + repos: { getContent: mockGetContent }, + }, + } as unknown as ReturnType); + + const result = await compareWithBase(resultWithUnsafePaths, "token"); + + expect(result).toEqual([ + { + file: "..", + baseSize: null, + currentSize: 3000, + change: null, + isNew: true, + }, + ]); + expect(mockGetContent).not.toHaveBeenCalled(); + expect(warning).toHaveBeenCalledWith( + "Skipping unsafe base-compare path: ../secrets.env" + ); + expect(warning).toHaveBeenCalledWith( + "Skipping unsafe base-compare path: .." + ); + }); + test("handles new file (404 error)", async () => { (context as { payload: Record }).payload = { pull_request: { base: { ref: "main" } }, diff --git a/packages/action/__tests__/inputs.test.ts b/packages/action/__tests__/inputs.test.ts index 55a5f8ae8..a763ee63d 100644 --- a/packages/action/__tests__/inputs.test.ts +++ b/packages/action/__tests__/inputs.test.ts @@ -315,6 +315,21 @@ describe("parseInputs auto mode", () => { expect(() => parseInputs()).not.toThrow(); }); + test("does not require type for esbuild when auto=true", () => { + vi.mocked(getInput).mockImplementation((name: string) => { + if (name === "compressor") return "esbuild"; + if (name === "type") return ""; + if (name === "output-dir") return "dist"; + return ""; + }); + vi.mocked(getBooleanInput).mockImplementation((name: string) => { + if (name === "auto") return true; + return false; + }); + + expect(() => parseInputs()).not.toThrow(); + }); + test("parses patterns from comma-separated string", () => { vi.mocked(getInput).mockImplementation((name: string) => { if (name === "patterns") return "src/**/*.js, lib/**/*.ts"; diff --git a/packages/action/__tests__/minify.test.ts b/packages/action/__tests__/minify.test.ts index 242dd4d4b..9150c0477 100644 --- a/packages/action/__tests__/minify.test.ts +++ b/packages/action/__tests__/minify.test.ts @@ -1,6 +1,7 @@ /*! node-minify action tests - MIT Licensed */ import { stat } from "node:fs/promises"; +import path from "node:path"; import { minify } from "@node-minify/core"; import { getFilesizeGzippedRaw, resolveCompressor } from "@node-minify/utils"; import { beforeEach, describe, expect, test, vi } from "vitest"; @@ -66,6 +67,7 @@ describe("runMinification", () => { files: [ { file: "src/app.js", + outputFile: "dist/app.min.js", originalSize: 1000, minifiedSize: 500, reduction: 50, @@ -171,4 +173,44 @@ describe("runMinification", () => { }) ); }); + + test("should store outputFile as repository-relative path for absolute output", async () => { + vi.mocked(stat) + .mockResolvedValueOnce({ size: 1000 } as any) + .mockResolvedValueOnce({ size: 500 } as any); + vi.mocked(resolveCompressor).mockResolvedValue({ + compressor: vi.fn(), + label: "terser", + } as any); + vi.mocked(minify).mockResolvedValue("minified content"); + + const absoluteOutput = path.resolve(process.cwd(), "dist/app.min.js"); + const result = await runMinification({ + ...mockInputs, + output: absoluteOutput, + }); + + expect(result.files[0]?.outputFile).toBe("dist/app.min.js"); + }); + + test("should include working-directory prefix in outputFile", async () => { + vi.mocked(stat) + .mockResolvedValueOnce({ size: 1000 } as any) + .mockResolvedValueOnce({ size: 500 } as any); + vi.mocked(resolveCompressor).mockResolvedValue({ + compressor: vi.fn(), + label: "terser", + } as any); + vi.mocked(minify).mockResolvedValue("minified content"); + + const result = await runMinification({ + ...mockInputs, + workingDirectory: "packages/action", + output: "dist/app.min.js", + }); + + expect(result.files[0]?.outputFile).toBe( + "packages/action/dist/app.min.js" + ); + }); }); diff --git a/packages/action/__tests__/runAutoMode.test.ts b/packages/action/__tests__/runAutoMode.test.ts index 74346935b..8f0d0e7e1 100644 --- a/packages/action/__tests__/runAutoMode.test.ts +++ b/packages/action/__tests__/runAutoMode.test.ts @@ -3,21 +3,28 @@ import { mkdir, stat } from "node:fs/promises"; import path from "node:path"; import * as core from "@actions/core"; +import { context } from "@actions/github"; import { minify } from "@node-minify/core"; import { getFilesizeGzippedRaw, resolveCompressor } from "@node-minify/utils"; import { beforeEach, describe, expect, test, vi } from "vitest"; +import { addAnnotations } from "../src/annotations.ts"; import { groupFilesByType, selectCompressor } from "../src/autoDetect.ts"; import { checkThresholds } from "../src/checks.ts"; +import { postPRComment } from "../src/comment.ts"; +import { compareWithBase } from "../src/compare.ts"; import { discoverFiles, generateOutputPath } from "../src/discover.ts"; import { runAutoMode } from "../src/index.ts"; import { setMinifyOutputs } from "../src/outputs.ts"; -import { generateSummary } from "../src/reporters/summary.ts"; +import { generateAutoModeSummary } from "../src/reporters/summary.ts"; vi.mock("@actions/core"); vi.mock("@actions/github"); vi.mock("node:fs/promises"); +vi.mock("../src/annotations.ts"); vi.mock("../src/discover.ts"); vi.mock("../src/autoDetect.ts"); +vi.mock("../src/comment.ts"); +vi.mock("../src/compare.ts"); vi.mock("@node-minify/utils"); vi.mock("@node-minify/core"); vi.mock("../src/outputs.ts"); @@ -45,11 +52,12 @@ describe("runAutoMode", () => { options: {}, benchmark: false, benchmarkCompressors: [], - githubToken: undefined, + githubToken: "token", }; beforeEach(() => { vi.clearAllMocks(); + (context as { payload: Record }).payload = {}; vi.mocked(stat).mockResolvedValue({ size: 100 } as any); vi.mocked(resolveCompressor).mockResolvedValue({ compressor: vi.fn(), @@ -64,6 +72,7 @@ describe("runAutoMode", () => { ); vi.mocked(minify).mockResolvedValue("minified content"); vi.mocked(checkThresholds).mockReturnValue(null); + vi.mocked(compareWithBase).mockResolvedValue([]); vi.mocked(groupFilesByType).mockReturnValue({ js: [], css: [], @@ -314,7 +323,7 @@ describe("runAutoMode", () => { ); }); - test("should call generateSummary when reportSummary is true", async () => { + test("should call generateAutoModeSummary when reportSummary is true", async () => { vi.mocked(discoverFiles).mockReturnValue(["file1.js"]); vi.mocked(groupFilesByType).mockReturnValue({ js: ["file1.js"], @@ -327,10 +336,10 @@ describe("runAutoMode", () => { await runAutoMode({ ...mockInputs, reportSummary: true }); - expect(generateSummary).toHaveBeenCalled(); + expect(generateAutoModeSummary).toHaveBeenCalled(); }); - test("should not call generateSummary when reportSummary is false", async () => { + test("should not call generateAutoModeSummary when reportSummary is false", async () => { vi.mocked(discoverFiles).mockReturnValue(["file1.js"]); vi.mocked(groupFilesByType).mockReturnValue({ js: ["file1.js"], @@ -343,7 +352,7 @@ describe("runAutoMode", () => { await runAutoMode({ ...mockInputs, reportSummary: false }); - expect(generateSummary).not.toHaveBeenCalled(); + expect(generateAutoModeSummary).not.toHaveBeenCalled(); }); test("should call setFailed when threshold check fails", async () => { @@ -381,4 +390,90 @@ describe("runAutoMode", () => { recursive: true, }); }); + + test("should store repository-relative outputFile in auto mode", async () => { + vi.mocked(discoverFiles).mockReturnValue(["file1.js"]); + vi.mocked(groupFilesByType).mockReturnValue({ + js: ["file1.js"], + css: [], + html: [], + json: [], + svg: [], + unknown: [], + }); + + await runAutoMode({ + ...mockInputs, + workingDirectory: "src", + }); + + expect(setMinifyOutputs).toHaveBeenCalledWith( + expect.objectContaining({ + files: [ + expect.objectContaining({ + file: "file1.js", + outputFile: "src/min/file1.js", + }), + ], + }) + ); + }); + + test("should post PR comment in auto mode when enabled in PR context", async () => { + vi.mocked(discoverFiles).mockReturnValue(["file1.js"]); + vi.mocked(groupFilesByType).mockReturnValue({ + js: ["file1.js"], + css: [], + html: [], + json: [], + svg: [], + unknown: [], + }); + (context as { payload: Record }).payload = { + pull_request: { number: 123 }, + }; + const comparisons = [ + { + file: "file1.js", + baseSize: 120, + currentSize: 100, + change: -16.7, + isNew: false, + }, + ]; + vi.mocked(compareWithBase).mockResolvedValue(comparisons); + + await runAutoMode({ + ...mockInputs, + reportPRComment: true, + }); + + expect(compareWithBase).toHaveBeenCalled(); + expect(postPRComment).toHaveBeenCalledWith( + expect.objectContaining({ compressor: "auto" }), + "token", + comparisons + ); + }); + + test("should add annotations in auto mode when enabled", async () => { + vi.mocked(discoverFiles).mockReturnValue(["file1.js"]); + vi.mocked(groupFilesByType).mockReturnValue({ + js: ["file1.js"], + css: [], + html: [], + json: [], + svg: [], + unknown: [], + }); + + await runAutoMode({ + ...mockInputs, + reportAnnotations: true, + }); + + expect(addAnnotations).toHaveBeenCalledWith( + expect.objectContaining({ compressor: "auto" }) + ); + }); }); diff --git a/packages/action/src/compare.ts b/packages/action/src/compare.ts index 2c54723f8..e118bb779 100644 --- a/packages/action/src/compare.ts +++ b/packages/action/src/compare.ts @@ -4,10 +4,36 @@ * MIT Licensed */ +import path from "node:path"; import { info, warning } from "@actions/core"; import { context, getOctokit } from "@actions/github"; import type { ComparisonResult, MinifyResult } from "./types.ts"; +/** + * Normalize and validate a repository path for GitHub content API lookups. + * + * @param candidate - Path to validate + * @returns Safe repository-relative path or null when invalid + */ +function normalizeComparePath(candidate: string): string | null { + const withForwardSlashes = candidate.replace(/\\/g, "/"); + const normalized = path.posix.normalize(withForwardSlashes); + + // GitHub API expects repository-relative paths. + if ( + normalized === "" || + normalized === "." || + normalized === ".." || + normalized.startsWith("../") || + normalized.startsWith("/") || + /^[a-zA-Z]:\//.test(normalized) + ) { + return null; + } + + return normalized; +} + /** * Compares minified file sizes against the base branch for pull requests. * @@ -39,16 +65,39 @@ export async function compareWithBase( const { owner, repo } = context.repo; const results = await Promise.all( - result.files.map((fileResult) => - compareFile( + result.files.map((fileResult) => { + const normalizedOutputPath = fileResult.outputFile + ? normalizeComparePath(fileResult.outputFile) + : null; + if (fileResult.outputFile && normalizedOutputPath === null) { + warning( + `Skipping unsafe base-compare path: ${fileResult.outputFile}` + ); + } + + const fallbackPath = normalizeComparePath(fileResult.file); + const comparePath = normalizedOutputPath ?? fallbackPath; + if (!comparePath) { + warning(`Skipping unsafe base-compare path: ${fileResult.file}`); + return { + file: fileResult.file, + baseSize: null, + currentSize: fileResult.minifiedSize, + change: null, + isNew: true, + } satisfies ComparisonResult; + } + + return compareFile( octokit, owner, repo, baseBranch, fileResult.file, + comparePath, fileResult.minifiedSize - ) - ) + ); + }) ); return results; @@ -61,7 +110,8 @@ export async function compareWithBase( * @param owner - Repository owner * @param repo - Repository name * @param baseBranch - Base branch ref (e.g., "main") - * @param filePath - Path to the file to compare + * @param sourceFilePath - Source file path used as display key in reports + * @param comparePath - Output file path used to query the base branch content * @param currentSize - Current minified size in bytes * @returns Comparison result with base size (null if file is new) and change percentage */ @@ -70,14 +120,15 @@ async function compareFile( owner: string, repo: string, baseBranch: string, - filePath: string, + sourceFilePath: string, + comparePath: string, currentSize: number ): Promise { try { const { data } = await octokit.rest.repos.getContent({ owner, repo, - path: filePath, + path: comparePath, ref: baseBranch, }); @@ -86,7 +137,7 @@ async function compareFile( if (Array.isArray(data)) { // Path is a directory, not a file return { - file: filePath, + file: sourceFilePath, baseSize: null, currentSize, change: null, @@ -97,7 +148,7 @@ async function compareFile( if (data.type !== "file" || !("size" in data)) { // Not a regular file (could be a symlink or submodule) return { - file: filePath, + file: sourceFilePath, baseSize: null, currentSize, change: null, @@ -114,7 +165,7 @@ async function compareFile( : 0; return { - file: filePath, + file: sourceFilePath, baseSize, currentSize, change, @@ -128,7 +179,7 @@ async function compareFile( (error as { status: number }).status === 404 ) { return { - file: filePath, + file: sourceFilePath, baseSize: null, currentSize, change: null, @@ -138,11 +189,11 @@ async function compareFile( // Log unexpected errors but don't fail the action warning( - `Failed to fetch base branch version of ${filePath}: ${error instanceof Error ? error.message : String(error)}` + `Failed to fetch base branch version of ${comparePath}: ${error instanceof Error ? error.message : String(error)}` ); return { - file: filePath, + file: sourceFilePath, baseSize: null, currentSize, change: null, diff --git a/packages/action/src/index.ts b/packages/action/src/index.ts index bb285a626..2d75ffd6e 100644 --- a/packages/action/src/index.ts +++ b/packages/action/src/index.ts @@ -27,6 +27,7 @@ import { parseInputs, validateCompressor } from "./inputs.ts"; import { runMinification } from "./minify.ts"; import { setBenchmarkOutputs, setMinifyOutputs } from "./outputs.ts"; import { + generateAutoModeSummary, generateBenchmarkSummary, generateSummary, } from "./reporters/summary.ts"; @@ -60,6 +61,13 @@ async function getFileSize(filePath: string): Promise { return stats.size; } +/** + * Convert an absolute filesystem path to a repository-relative path. + */ +function toRepositoryPath(filePath: string): string { + return path.relative(process.cwd(), filePath).replace(/\\/g, "/"); +} + /** * Runs the explicit mode minification workflow (original behavior). * @@ -231,6 +239,7 @@ export async function runAutoMode(inputs: ActionInputs): Promise { return { file, + outputFile: toRepositoryPath(fullOutputPath), originalSize, minifiedSize, reduction, @@ -270,7 +279,6 @@ export async function runAutoMode(inputs: ActionInputs): Promise { return; } - // Generate summary using existing function (stub for now - Task 6 will implement generateAutoModeSummary) const totalOriginalSize = allResults.reduce( (sum, r) => sum + r.originalSize, 0 @@ -298,7 +306,19 @@ export async function runAutoMode(inputs: ActionInputs): Promise { setMinifyOutputs(minifyResult); if (inputs.reportSummary) { - await generateSummary(minifyResult); + await generateAutoModeSummary([minifyResult], inputs); + } + + if (inputs.reportPRComment && context.payload.pull_request) { + const comparisons = await compareWithBase( + minifyResult, + inputs.githubToken + ); + await postPRComment(minifyResult, inputs.githubToken, comparisons); + } + + if (inputs.reportAnnotations) { + addAnnotations(minifyResult); } const thresholdError = checkThresholds(minifyResult.totalReduction, inputs); diff --git a/packages/action/src/inputs.ts b/packages/action/src/inputs.ts index 7d23a3c9c..4f40603e7 100644 --- a/packages/action/src/inputs.ts +++ b/packages/action/src/inputs.ts @@ -98,7 +98,7 @@ export function parseInputs(): ActionInputs { type = typeRaw; } - if (TYPE_REQUIRED_COMPRESSORS.includes(compressor) && !type) { + if (!auto && TYPE_REQUIRED_COMPRESSORS.includes(compressor) && !type) { throw new Error( `Compressor '${compressor}' requires the 'type' input (js or css)` ); diff --git a/packages/action/src/minify.ts b/packages/action/src/minify.ts index 153fb1e16..84ca27dc3 100644 --- a/packages/action/src/minify.ts +++ b/packages/action/src/minify.ts @@ -5,7 +5,7 @@ */ import { stat } from "node:fs/promises"; -import { resolve } from "node:path"; +import { relative, resolve } from "node:path"; import { minify } from "@node-minify/core"; import { getFilesizeGzippedRaw, resolveCompressor } from "@node-minify/utils"; import type { ActionInputs, FileResult, MinifyResult } from "./types.ts"; @@ -21,6 +21,16 @@ async function getFileSize(filePath: string): Promise { return stats.size; } +/** + * Convert an absolute filesystem path to a repository-relative path. + * + * @param filePath - Absolute path to normalize + * @returns Repository-relative path using POSIX separators + */ +function toRepositoryPath(filePath: string): string { + return relative(process.cwd(), filePath).replace(/\\/g, "/"); +} + /** * Minifies a single input file according to the provided action inputs and returns summary metrics. * @@ -79,6 +89,7 @@ export async function runMinification( const fileResult: FileResult = { file: input, + outputFile: toRepositoryPath(outputPath), originalSize, minifiedSize, reduction, diff --git a/packages/action/src/types.ts b/packages/action/src/types.ts index 9701a213d..b9f5bb30d 100644 --- a/packages/action/src/types.ts +++ b/packages/action/src/types.ts @@ -61,6 +61,7 @@ export interface ActionInputs { export interface FileResult { file: string; + outputFile?: string; originalSize: number; minifiedSize: number; reduction: number;