fix: preserve JSON comments in firmware submission workflow (#292)

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: AlCalzone

.github/scripts/firmware-submission/process-submission.mts

@@ -1,5 +1,9 @@
 import * as githubActions from "@actions/github";
 import { downloadFirmware, generateHash } from "@zwave-js/firmware-integrity";
+import {
+	parse as parseCommentJson,
+	stringify as stringifyCommentJson,
+} from "comment-json";
 import JSON5 from "json5";
 import { spawnSync } from "node:child_process";
 import * as fs from "node:fs";
@@ -284,7 +288,11 @@ function buildIssueFieldHeadingSequences(): string[][] {
 		"single-target-chip-1-with-target",
 	];
 	for (let deviceCount = 1; deviceCount <= MAX_DEVICES; deviceCount++) {
-		for (let upgradeCount = 1; upgradeCount <= MAX_UPGRADES; upgradeCount++) {
+		for (
+			let upgradeCount = 1;
+			upgradeCount <= MAX_UPGRADES;
+			upgradeCount++
+		) {
 			for (const fileLayout of fileLayouts) {
 				sequences.add(
 					JSON.stringify(
@@ -446,6 +454,33 @@ function loadFirmwareConfigs(): FirmwareConfigFile[] {
 	});
 }
 
+export function appendUpgradesToFirmwareConfigText(
+	configText: string,
+	newUpgrades: readonly Record<string, any>[],
+): string {
+	const config = parseCommentJson<Record<string, any>>(configText);
+	if (!Array.isArray(config.upgrades)) {
+		throw new Error("Firmware config does not contain an upgrades array.");
+	}
+
+	for (const upgrade of newUpgrades) {
+		config.upgrades.push(upgrade);
+	}
+
+	return `${stringifyCommentJson(config, null, "\t")}\n`;
+}
+
+export async function formatWithPrettier(
+	text: string,
+	parser: string,
+	prettierConfig: Record<string, any> = {},
+): Promise<string> {
+	return prettier.format(text, {
+		...prettierConfig,
+		parser,
+	});
+}
+
 function chooseExistingDirectory(
 	candidates: string[],
 	preferredDirectory: string,
@@ -616,7 +651,8 @@ export function parseIssueBody(body: string): Record<string, string | null> {
 			? candidateSequences
 					.filter(
 						(candidate) =>
-							candidate.headings[candidate.nextHeadingIndex] === heading,
+							candidate.headings[candidate.nextHeadingIndex] ===
+							heading,
 					)
 					.map((candidate) => ({
 						headings: candidate.headings,
@@ -888,7 +924,11 @@ function normalizeUpgradeVariant(upgrade: Record<string, any>): {
 function getUpgradeVariantKey(
 	variant: NonNullable<ReturnType<typeof normalizeUpgradeVariant>>,
 ): string {
-	return JSON.stringify([variant.version, variant.region, variant.ifCondition]);
+	return JSON.stringify([
+		variant.version,
+		variant.region,
+		variant.ifCondition,
+	]);
 }
 
 function describeUpgradeVariant(
@@ -1030,21 +1070,15 @@ export function parseUpgradeFilesFromSections({
 					? getSingleTargetTargetNumberFieldLabel(upgradeIndex)
 					: descriptor.targetLabels[0],
 		});
-		if (
-			urlLabel === singleTargetUrlLabel &&
-			targetRaw != null
-		) {
+		if (urlLabel === singleTargetUrlLabel && targetRaw != null) {
 			errors.push(
 				`'${targetLabel}' is not supported in the single-target submission form. That form always uses target number 0. Use the 'Firmware Submission' form instead.`,
 			);
 		}
 		const target =
 			targetRaw != null && urlLabel !== singleTargetUrlLabel
-				? (validateTargetNumber(
-						targetRaw,
-						targetLabel,
-						errors,
-					) ?? descriptor.defaultTarget)
+				? (validateTargetNumber(targetRaw, targetLabel, errors) ??
+					descriptor.defaultTarget)
 				: descriptor.defaultTarget;
 
 		files.push({ target, url });
@@ -1761,10 +1795,11 @@ export default async function main({
 		for (const upgrade of upgradeFormData) {
 			const raw = upgrade.changelog.trim().replace(/\r\n/g, "\n");
 			try {
-				const formatted = await prettier.format(raw, {
-					...prettierConfig,
-					parser: "markdown",
-				});
+				const formatted = await formatWithPrettier(
+					raw,
+					"markdown",
+					prettierConfig,
+				);
 				formattedChangelogs.push(formatted.trim());
 			} catch {
 				formattedChangelogs.push(raw);
@@ -1801,42 +1836,44 @@ export default async function main({
 			]);
 		}
 
-		const config = existingConfig
-			? {
-					...existingConfig,
-					upgrades: [
-						...(existingConfig.upgrades ?? []),
-						...newUpgrades,
-					],
-				}
-			: {
-					devices: devices.map((device) => {
-						const entry: Record<string, any> = {
-							brand: device.brand,
-							model: device.model,
-							manufacturerId: device.manufacturerId,
-							productType: device.productType,
-							productId: device.productId,
-						};
-						if (device.firmwareVersion) {
-							entry.firmwareVersion = device.firmwareVersion;
-						}
-						return entry;
-					}),
-					upgrades: newUpgrades,
-				};
+		const configText = matchedExistingFile
+			? appendUpgradesToFirmwareConfigText(
+					fs.readFileSync(matchedExistingFile.absolutePath, "utf-8"),
+					newUpgrades,
+				)
+			: `${stringifyCommentJson(
+					{
+						devices: devices.map((device) => {
+							const entry: Record<string, any> = {
+								brand: device.brand,
+								model: device.model,
+								manufacturerId: device.manufacturerId,
+								productType: device.productType,
+								productId: device.productId,
+							};
+							if (device.firmwareVersion) {
+								entry.firmwareVersion = device.firmwareVersion;
+							}
+							return entry;
+						}),
+						upgrades: newUpgrades,
+					},
+					null,
+					"\t",
+				)}\n`;
+		const formattedConfigText = await formatWithPrettier(
+			configText,
+			"jsonc",
+			prettierConfig,
+		);
 
 		console.log(
 			"Re-checking approved issue state before writing changes...",
 		);
 		await ensureIssueStillApproved(true);
 		console.log(`Writing config to ${relativeFilePath}...`);
 		fs.mkdirSync(path.dirname(absoluteFilePath), { recursive: true });
-		fs.writeFileSync(
-			absoluteFilePath,
-			`${JSON.stringify(config, null, "\t")}\n`,
-			"utf-8",
-		);
+		fs.writeFileSync(absoluteFilePath, formattedConfigText, "utf-8");
 
 		git("add", relativeFilePath);
 		const lastVersion =

package.json

@@ -37,6 +37,7 @@
     "@typescript-eslint/parser": "^8.43.0",
     "@zwave-js/firmware-integrity": "^1.0.2",
     "ava": "^4.3.3",
+    "comment-json": "^4.6.2",
     "esbuild": "^0.15.7",
     "esbuild-register": "^3.3.3",
     "eslint": "^9.35.0",

test/firmware-submission-regressions.ts

@@ -1,4 +1,5 @@
 import test from "ava";
+import JSON5 from "json5";
 import { readFile } from "node:fs/promises";
 
 const processSubmissionModulePath =
@@ -16,10 +17,12 @@ const resetOnEditModule = await import(resetOnEditModulePath);
 const cleanupLabelsModule = await import(cleanupLabelsModulePath);
 
 const {
+	appendUpgradesToFirmwareConfigText,
 	createUpgradeEntry,
 	extractIssueTemplateFieldHeadings,
 	findDuplicateTargets,
 	findDuplicateUpgradeVariants,
+	formatWithPrettier,
 	getApprovalInvalidReason,
 	parseIssueBody,
 	parseUpgradeFilesFromSections,
@@ -692,6 +695,68 @@ test("createUpgradeEntry preserves submitted file order and targets", (t) => {
 	);
 });
 
+test("appendUpgradesToFirmwareConfigText preserves existing JSONC comments after formatting", async (t) => {
+	const existingConfig = `{
+	"devices": [
+		{
+			"brand": "Zooz",
+			"model": "ZEN51", // existing model comment
+			"manufacturerId": "0x027a",
+			"productType": "0x7000",
+			"productId": "0xa008"
+		}
+	],
+	"upgrades": [
+		// existing upgrade comment
+		{
+			"version": "1.60",
+			"changelog": "Existing release",
+			"url": "https://example.com/existing.gbl",
+			"integrity": "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+		}
+	]
+}
+`;
+
+	const updatedConfig = await formatWithPrettier(
+		appendUpgradesToFirmwareConfigText(existingConfig, [
+			createUpgradeEntry({
+				version: "1.61",
+				changelog: "New release",
+				channel: "stable",
+				region: null,
+				ifCondition: null,
+				files: [
+					{
+						target: 0,
+						url: "https://example.com/new.gbl",
+						integrity:
+							"sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+					},
+				],
+			}),
+			]),
+		"jsonc",
+		{
+			endOfLine: "lf",
+			tabWidth: 4,
+			useTabs: true,
+		},
+	);
+
+	t.true(updatedConfig.includes("// existing model comment"));
+	t.true(updatedConfig.includes("// existing upgrade comment"));
+	t.true(updatedConfig.includes('\t"devices": ['));
+
+	const parsed: {
+		upgrades: Array<{ version: string }>;
+	} = JSON5.parse(updatedConfig);
+	t.deepEqual(
+		parsed.upgrades.map((upgrade) => upgrade.version),
+		["1.60", "1.61"],
+	);
+});
+
 test("createUpgradeEntry uses top-level url and integrity for a single target 0 file", (t) => {
 	const entry = createUpgradeEntry({
 		version: "1.61",

yarn.lock

@@ -1068,6 +1068,7 @@ __metadata:
     "@typescript-eslint/parser": ^8.43.0
     "@zwave-js/firmware-integrity": ^1.0.2
     ava: ^4.3.3
+    comment-json: ^4.6.2
     esbuild: ^0.15.7
     esbuild-register: ^3.3.3
     eslint: ^9.35.0
@@ -1289,6 +1290,13 @@ __metadata:
   languageName: node
   linkType: hard
 
+"array-timsort@npm:^1.0.3":
+  version: 1.0.3
+  resolution: "array-timsort@npm:1.0.3"
+  checksum: fd4b5b0911214bdc8b5699ed10d309685551b518b3819c611c967cff59b87aee01cf591a10e36a3f14dbff696984bd6682b845f6fdbf1217195e910f241a4f78
+  languageName: node
+  linkType: hard
+
 "array-union@npm:^2.1.0":
   version: 2.1.0
   resolution: "array-union@npm:2.1.0"
@@ -1738,6 +1746,16 @@ __metadata:
   languageName: node
   linkType: hard
 
+"comment-json@npm:^4.6.2":
+  version: 4.6.2
+  resolution: "comment-json@npm:4.6.2"
+  dependencies:
+    array-timsort: ^1.0.3
+    esprima: ^4.0.1
+  checksum: d235957843363c562ce6df85dd17cabe908e515ff8d7641cc90fdda624c1d3b73a85aec1bfe6a6601c3cc6ce8132aaa5dc35f99b0404d26c0a1f119fe97534ff
+  languageName: node
+  linkType: hard
+
 "common-path-prefix@npm:^3.0.0":
   version: 3.0.0
   resolution: "common-path-prefix@npm:3.0.0"
@@ -2509,7 +2527,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"esprima@npm:^4.0.0":
+"esprima@npm:^4.0.0, esprima@npm:^4.0.1":
   version: 4.0.1
   resolution: "esprima@npm:4.0.1"
   bin: