fix: address review — convert JS to TS, add tests, eliminate duplicate CI runs

- Convert coverage-report.js and unified-report.js to TypeScript
- Add 37 unit tests for coverage-slack-notify.ts pure functions
- Export pure functions from coverage-slack-notify.ts for testability
- Eliminate duplicate unit test run: Slack workflow now downloads
  coverage artifacts from CI: Tests Unit via workflow_run trigger
  instead of re-running pnpm test:coverage
- Upload unit-coverage artifact from ci-tests-unit.yaml on push
- Remove E2E coverage from PR report (only runs on push to main,
  cannot populate PR comments) — keep as main-only baseline
- Remove dead coverage-status arg from unified-report.ts
- Fix pr-meta in Slack workflow to read commit from workflow_run
  context instead of push payload

.github/workflows/ci-tests-unit.yaml

@@ -26,10 +26,20 @@ jobs:
       - name: Run Vitest tests with coverage
         run: pnpm test:coverage
 
+      - name: Upload unit coverage artifact
+        if: always() && github.event_name == 'push'
+        uses: actions/upload-artifact@v6
+        with:
+          name: unit-coverage
+          path: coverage/lcov.info
+          retention-days: 30
+          if-no-files-found: warn
+
       - name: Upload coverage to Codecov
         if: always()
         uses: codecov/codecov-action@1af58845a975a7985b0beb0cbe6fbbb71a41dbad # v5.5.3
         with:
           files: coverage/lcov.info
+          flags: unit
           token: ${{ secrets.CODECOV_TOKEN }}
           fail_ci_if_error: false

.github/workflows/coverage-slack-notify.yaml

@@ -1,9 +1,11 @@
 name: 'Coverage: Slack Notification'
 
 on:
-  push:
+  workflow_run:
+    workflows: ['CI: Tests Unit']
     branches: [main]
-    paths-ignore: ['**/*.md']
+    types:
+      - completed
 
 permissions:
   contents: read
@@ -12,18 +14,23 @@ permissions:
 
 jobs:
   notify:
-    if: github.repository == 'Comfy-Org/ComfyUI_frontend'
+    if: >
+      github.repository == 'Comfy-Org/ComfyUI_frontend' &&
+      github.event.workflow_run.conclusion == 'success' &&
+      github.event.workflow_run.event == 'push'
     runs-on: ubuntu-latest
-
     steps:
       - uses: actions/checkout@v6
 
       - name: Setup frontend
         uses: ./.github/actions/setup-frontend
 
-      - name: Run unit tests with coverage
-        id: unit-tests
-        run: pnpm test:coverage
+      - name: Download current unit coverage
+        uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12
+        with:
+          run_id: ${{ github.event.workflow_run.id }}
+          name: unit-coverage
+          path: coverage
 
       - name: Download previous unit coverage baseline
         continue-on-error: true
@@ -60,7 +67,13 @@ jobs:
         uses: actions/github-script@v8
         with:
           script: |
-            const message = context.payload.head_commit?.message ?? '';
+            const sha = context.payload.workflow_run.head_sha;
+            const { data: commit } = await github.rest.repos.getCommit({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              ref: sha,
+            });
+            const message = commit.commit.message ?? '';
             const firstLine = message.split('\n')[0];
             const match = firstLine.match(/\(#(\d+)\)\s*$/);
             if (!match) {
@@ -118,7 +131,7 @@ jobs:
             https://slack.com/api/chat.postMessage
 
       - name: Save unit coverage baseline
-        if: always() && steps.unit-tests.outcome == 'success'
+        if: always() && hashFiles('coverage/lcov.info') != ''
         uses: actions/upload-artifact@v6
         with:
           name: unit-coverage-baseline

.github/workflows/pr-report.yaml

@@ -2,7 +2,7 @@ name: 'PR: Unified Report'
 
 on:
   workflow_run:
-    workflows: ['CI: Size Data', 'CI: Performance Report', 'CI: E2E Coverage']
+    workflows: ['CI: Size Data', 'CI: Performance Report']
     types:
       - completed
 
@@ -104,25 +104,6 @@ jobs:
           path: temp/size-prev
           if_no_artifact_found: warn
 
-      - name: Find coverage workflow run
-        if: steps.pr-meta.outputs.skip != 'true'
-        id: find-coverage
-        uses: ./.github/actions/find-workflow-run
-        with:
-          workflow-id: ci-tests-e2e-coverage.yaml
-          head-sha: ${{ steps.pr-meta.outputs.head-sha }}
-          not-found-status: skip
-          token: ${{ secrets.GITHUB_TOKEN }}
-
-      - name: Download coverage data
-        if: steps.pr-meta.outputs.skip != 'true' && steps.find-coverage.outputs.status == 'ready'
-        uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12
-        with:
-          name: e2e-coverage
-          run_id: ${{ steps.find-coverage.outputs.run-id }}
-          path: temp/coverage
-          if_no_artifact_found: warn
-
       - name: Download perf metrics (current)
         if: steps.pr-meta.outputs.skip != 'true' && steps.find-perf.outputs.status == 'ready'
         uses: dawidd6/action-download-artifact@0bd50d53a6d7fb5cb921e607957e9cc12b4ce392 # v12
@@ -158,10 +139,9 @@ jobs:
       - name: Generate unified report
         if: steps.pr-meta.outputs.skip != 'true'
         run: >
-          node scripts/unified-report.js
+          pnpm exec tsx scripts/unified-report.ts
           --size-status=${{ steps.find-size.outputs.status }}
           --perf-status=${{ steps.find-perf.outputs.status }}
-          --coverage-status=${{ steps.find-coverage.outputs.status }}
           > pr-report.md
 
       - name: Remove legacy separate comments

scripts/coverage-report.ts

@@ -1,6 +1,16 @@
-// @ts-check
 import { existsSync, readFileSync } from 'node:fs'
 
+interface FileStats {
+  lines: number
+  covered: number
+}
+
+interface UncoveredFile {
+  file: string
+  pct: number
+  missed: number
+}
+
 const lcovPath = process.argv[2] || 'coverage/playwright/coverage.lcov'
 
 if (!existsSync(lcovPath)) {
@@ -19,8 +29,7 @@ let coveredFunctions = 0
 let totalBranches = 0
 let coveredBranches = 0
 
-/** @type {Map<string, { lines: number, covered: number }>} */
-const fileStats = new Map()
+const fileStats = new Map<string, FileStats>()
 let currentFile = ''
 
 for (const line of lcov.split('\n')) {
@@ -49,14 +58,12 @@ for (const line of lcov.split('\n')) {
   }
 }
 
-/** @param {number} covered @param {number} total */
-function pct(covered, total) {
+function pct(covered: number, total: number): string {
   if (total === 0) return '—'
   return ((covered / total) * 100).toFixed(1) + '%'
 }
 
-/** @param {number} covered @param {number} total */
-function bar(covered, total) {
+function bar(covered: number, total: number): string {
   if (total === 0) return '—'
   const p = (covered / total) * 100
   if (p >= 80) return '🟢'
@@ -64,7 +71,7 @@ function bar(covered, total) {
   return '🔴'
 }
 
-const lines = []
+const lines: string[] = []
 lines.push('## 🔬 E2E Coverage')
 lines.push('')
 lines.push('| Metric | Covered | Total | Pct | |')
@@ -79,7 +86,7 @@ lines.push(
   `| Branches | ${coveredBranches} | ${totalBranches} | ${pct(coveredBranches, totalBranches)} | ${bar(coveredBranches, totalBranches)} |`
 )
 
-const uncovered = [...fileStats.entries()]
+const uncovered: UncoveredFile[] = [...fileStats.entries()]
   .filter(([, s]) => s.lines > 0)
   .map(([file, s]) => ({
     file: file.replace(/^.*\/src\//, 'src/'),

scripts/coverage-slack-notify.test.ts

@@ -0,0 +1,273 @@
+import { describe, expect, it } from 'vitest'
+
+import type { CoverageData } from './coverage-slack-notify'
+import {
+  buildMilestoneBlock,
+  crossedMilestone,
+  formatCoverageRow,
+  formatDelta,
+  formatPct,
+  parseArgs,
+  parseLcovContent,
+  progressBar
+} from './coverage-slack-notify'
+
+describe('parseLcovContent', () => {
+  it('parses valid LCOV content', () => {
+    const content = ['LF:100', 'LH:75', 'end_of_record'].join('\n')
+    const result = parseLcovContent(content)
+    expect(result).toEqual({
+      percentage: 75,
+      totalLines: 100,
+      coveredLines: 75
+    })
+  })
+
+  it('sums lines across multiple records', () => {
+    const content = [
+      'LF:100',
+      'LH:50',
+      'end_of_record',
+      'LF:200',
+      'LH:150',
+      'end_of_record'
+    ].join('\n')
+    const result = parseLcovContent(content)
+    expect(result).toEqual({
+      percentage: (200 / 300) * 100,
+      totalLines: 300,
+      coveredLines: 200
+    })
+  })
+
+  it('returns null for empty content', () => {
+    expect(parseLcovContent('')).toBeNull()
+  })
+
+  it('returns null when total lines is zero', () => {
+    const content = 'LH:10\nend_of_record'
+    expect(parseLcovContent(content)).toBeNull()
+  })
+
+  it('treats malformed numeric values as zero', () => {
+    const content = ['LF:abc', 'LH:10', 'LF:50', 'end_of_record'].join('\n')
+    const result = parseLcovContent(content)
+    expect(result).toEqual({
+      percentage: 20,
+      totalLines: 50,
+      coveredLines: 10
+    })
+  })
+
+  it('ignores unrelated lines', () => {
+    const content = [
+      'TN:',
+      'SF:/some/file.ts',
+      'LF:200',
+      'LH:100',
+      'end_of_record'
+    ].join('\n')
+    const result = parseLcovContent(content)
+    expect(result).toEqual({
+      percentage: 50,
+      totalLines: 200,
+      coveredLines: 100
+    })
+  })
+})
+
+describe('progressBar', () => {
+  it('renders empty bar at 0%', () => {
+    expect(progressBar(0)).toBe('░'.repeat(20))
+  })
+
+  it('renders half-filled bar at 50%', () => {
+    const bar = progressBar(50)
+    expect(bar).toBe('█'.repeat(10) + '░'.repeat(10))
+  })
+
+  it('renders full bar at 100%', () => {
+    expect(progressBar(100)).toBe('█'.repeat(20))
+  })
+
+  it('clamps negative values to 0%', () => {
+    expect(progressBar(-10)).toBe('░'.repeat(20))
+  })
+
+  it('clamps values above 100% to 100%', () => {
+    expect(progressBar(150)).toBe('█'.repeat(20))
+  })
+
+  it('has consistent length of 20 characters', () => {
+    for (const pct of [0, 25, 50, 75, 100]) {
+      expect(progressBar(pct)).toHaveLength(20)
+    }
+  })
+})
+
+describe('formatPct', () => {
+  it('formats integer percentage', () => {
+    expect(formatPct(50)).toBe('50.0%')
+  })
+
+  it('formats decimal percentage', () => {
+    expect(formatPct(12.34)).toBe('12.3%')
+  })
+
+  it('formats zero', () => {
+    expect(formatPct(0)).toBe('0.0%')
+  })
+
+  it('formats 100', () => {
+    expect(formatPct(100)).toBe('100.0%')
+  })
+})
+
+describe('formatDelta', () => {
+  it('formats positive delta with + sign', () => {
+    expect(formatDelta(1.2)).toBe('+1.2%')
+  })
+
+  it('formats negative delta with - sign', () => {
+    expect(formatDelta(-0.5)).toBe('-0.5%')
+  })
+
+  it('formats zero delta with + sign', () => {
+    expect(formatDelta(0)).toBe('+0.0%')
+  })
+
+  it('formats large positive delta', () => {
+    expect(formatDelta(15.67)).toBe('+15.7%')
+  })
+})
+
+describe('crossedMilestone', () => {
+  it('detects crossing a 5% boundary', () => {
+    expect(crossedMilestone(23, 27)).toBe(25)
+  })
+
+  it('returns null when no boundary is crossed', () => {
+    expect(crossedMilestone(21, 24)).toBeNull()
+  })
+
+  it('detects exact boundary crossing', () => {
+    expect(crossedMilestone(24.9, 25)).toBe(25)
+  })
+
+  it('returns highest milestone when crossing multiple boundaries', () => {
+    expect(crossedMilestone(24, 31)).toBe(30)
+  })
+
+  it('returns null when values are equal', () => {
+    expect(crossedMilestone(25, 25)).toBeNull()
+  })
+
+  it('returns null when coverage decreases', () => {
+    expect(crossedMilestone(30, 28)).toBeNull()
+  })
+
+  it('detects crossing from 0', () => {
+    expect(crossedMilestone(0, 7)).toBe(5)
+  })
+})
+
+describe('buildMilestoneBlock', () => {
+  it('builds celebration block below target', () => {
+    const block = buildMilestoneBlock('Unit test', 50)
+    expect(block.type).toBe('section')
+    expect(block.text.type).toBe('mrkdwn')
+    expect(block.text.text).toContain('MILESTONE: Unit test coverage hit 50%')
+    expect(block.text.text).toContain('30 percentage points to go')
+  })
+
+  it('builds goal-reached block at target', () => {
+    const block = buildMilestoneBlock('Unit test', 80)
+    expect(block.text.text).toContain('GOAL REACHED')
+    expect(block.text.text).toContain('80%')
+    expect(block.text.text).toContain('✅')
+  })
+
+  it('builds goal-reached block above target', () => {
+    const block = buildMilestoneBlock('E2E test', 85)
+    expect(block.text.text).toContain('GOAL REACHED')
+    expect(block.text.text).toContain('85%')
+  })
+
+  it('uses singular "point" when 1 remaining', () => {
+    const block = buildMilestoneBlock('Unit test', 79)
+    expect(block.text.text).toContain('1 percentage point to go')
+  })
+})
+
+describe('parseArgs', () => {
+  it('parses all args', () => {
+    const argv = [
+      '--pr-url=https://github.com/org/repo/pull/42',
+      '--pr-number=42',
+      '--author=testuser'
+    ]
+    expect(parseArgs(argv)).toEqual({
+      prUrl: 'https://github.com/org/repo/pull/42',
+      prNumber: '42',
+      author: 'testuser'
+    })
+  })
+
+  it('returns empty strings for missing args', () => {
+    expect(parseArgs([])).toEqual({
+      prUrl: '',
+      prNumber: '',
+      author: ''
+    })
+  })
+
+  it('handles args with empty values', () => {
+    const argv = ['--pr-url=', '--pr-number=', '--author=']
+    expect(parseArgs(argv)).toEqual({
+      prUrl: '',
+      prNumber: '',
+      author: ''
+    })
+  })
+
+  it('ignores unknown args', () => {
+    const argv = ['--unknown=value', '--pr-number=99']
+    expect(parseArgs(argv)).toEqual({
+      prUrl: '',
+      prNumber: '99',
+      author: ''
+    })
+  })
+})
+
+describe('formatCoverageRow', () => {
+  it('formats a coverage comparison row', () => {
+    const current: CoverageData = {
+      percentage: 55,
+      totalLines: 1000,
+      coveredLines: 550
+    }
+    const baseline: CoverageData = {
+      percentage: 50,
+      totalLines: 1000,
+      coveredLines: 500
+    }
+    const row = formatCoverageRow('Unit', current, baseline)
+    expect(row).toBe('*Unit:*  50.0% → 55.0%  (+5.0%)')
+  })
+
+  it('formats a row with negative delta', () => {
+    const current: CoverageData = {
+      percentage: 48,
+      totalLines: 1000,
+      coveredLines: 480
+    }
+    const baseline: CoverageData = {
+      percentage: 50,
+      totalLines: 1000,
+      coveredLines: 500
+    }
+    const row = formatCoverageRow('E2E', current, baseline)
+    expect(row).toBe('*E2E:*  50.0% → 48.0%  (-2.0%)')
+  })
+})

scripts/coverage-slack-notify.ts

@@ -4,7 +4,7 @@ const TARGET = 80
 const MILESTONE_STEP = 5
 const BAR_WIDTH = 20
 
-interface CoverageData {
+export interface CoverageData {
   percentage: number
   totalLines: number
   coveredLines: number
@@ -18,7 +18,7 @@ interface SlackBlock {
   }
 }
 
-function parseLcovContent(content: string): CoverageData | null {
+export function parseLcovContent(content: string): CoverageData | null {
   let totalLines = 0
   let coveredLines = 0
 
@@ -44,23 +44,23 @@ function parseLcov(filePath: string): CoverageData | null {
   return parseLcovContent(readFileSync(filePath, 'utf-8'))
 }
 
-function progressBar(percentage: number): string {
+export function progressBar(percentage: number): string {
   const clamped = Math.max(0, Math.min(100, percentage))
   const filled = Math.round((clamped / 100) * BAR_WIDTH)
   const empty = BAR_WIDTH - filled
   return '█'.repeat(filled) + '░'.repeat(empty)
 }
 
-function formatPct(value: number): string {
+export function formatPct(value: number): string {
   return value.toFixed(1) + '%'
 }
 
-function formatDelta(delta: number): string {
+export function formatDelta(delta: number): string {
   const sign = delta >= 0 ? '+' : ''
   return sign + delta.toFixed(1) + '%'
 }
 
-function crossedMilestone(prev: number, curr: number): number | null {
+export function crossedMilestone(prev: number, curr: number): number | null {
   const prevBucket = Math.floor(prev / MILESTONE_STEP)
   const currBucket = Math.floor(curr / MILESTONE_STEP)
 
@@ -70,7 +70,10 @@ function crossedMilestone(prev: number, curr: number): number | null {
   return null
 }
 
-function buildMilestoneBlock(label: string, milestone: number): SlackBlock {
+export function buildMilestoneBlock(
+  label: string,
+  milestone: number
+): SlackBlock {
   if (milestone >= TARGET) {
     return {
       type: 'section',
@@ -99,7 +102,7 @@ function buildMilestoneBlock(label: string, milestone: number): SlackBlock {
   }
 }
 
-function parseArgs(argv: string[]): {
+export function parseArgs(argv: string[]): {
   prUrl: string
   prNumber: string
   author: string
@@ -118,7 +121,7 @@ function parseArgs(argv: string[]): {
   return { prUrl, prNumber, author }
 }
 
-function formatCoverageRow(
+export function formatCoverageRow(
   label: string,
   current: CoverageData,
   baseline: CoverageData
@@ -210,4 +213,6 @@ function main() {
   process.stdout.write(JSON.stringify(payload))
 }
 
-main()
+if (process.argv[1] === import.meta.filename) {
+  main()
+}

scripts/unified-report.ts

@@ -1,11 +1,9 @@
-// @ts-check
 import { execFileSync } from 'node:child_process'
 import { existsSync } from 'node:fs'
 
-const args = process.argv.slice(2)
+const args: string[] = process.argv.slice(2)
 
-/** @param {string} name */
-function getArg(name) {
+function getArg(name: string): string | undefined {
   const prefix = `--${name}=`
   const arg = args.find((a) => a.startsWith(prefix))
   return arg ? arg.slice(prefix.length) : undefined
@@ -13,10 +11,7 @@ function getArg(name) {
 
 const sizeStatus = getArg('size-status') ?? 'pending'
 const perfStatus = getArg('perf-status') ?? 'pending'
-const coverageStatus = getArg('coverage-status') ?? 'skip'
-
-/** @type {string[]} */
-const lines = []
+const lines: string[] = []
 
 if (sizeStatus === 'ready') {
   try {
@@ -71,28 +66,4 @@ if (perfStatus === 'ready' && existsSync('test-results/perf-metrics.json')) {
   lines.push('> ⏳ Performance tests in progress…')
 }
 
-if (coverageStatus === 'ready' && existsSync('temp/coverage/coverage.lcov')) {
-  try {
-    const coverageReport = execFileSync(
-      'node',
-      ['scripts/coverage-report.js', 'temp/coverage/coverage.lcov'],
-      { encoding: 'utf-8' }
-    ).trimEnd()
-    lines.push('')
-    lines.push(coverageReport)
-  } catch {
-    lines.push('')
-    lines.push('## 🔬 E2E Coverage')
-    lines.push('')
-    lines.push(
-      '> ⚠️ Failed to render coverage report. Check the CI workflow logs.'
-    )
-  }
-} else if (coverageStatus === 'failed') {
-  lines.push('')
-  lines.push('## 🔬 E2E Coverage')
-  lines.push('')
-  lines.push('> ⚠️ Coverage collection failed. Check the CI workflow logs.')
-}
-
 process.stdout.write(lines.join('\n') + '\n')