refactor(#14): Address PR feedback

README.md

@@ -126,7 +126,7 @@ If you prefer compose:
 services:
   open-network-diagram:
     image: jcreek23/open-network-diagram:latest
-    user: '${OND_UID:-1000}:${OND_GID:-1000}'
+    user: '${OND_UID:?Set OND_UID to your host user ID}:${OND_GID:?Set OND_GID to your host group ID}'
     ports:
       - '8080:3000'
     volumes:
@@ -144,7 +144,7 @@ mkdir -p data
 OND_UID="$(id -u)" OND_GID="$(id -g)" docker compose up -d
 ```
 
-`OND_UID` and `OND_GID` are used only by Compose to select the container process identity; they are not application environment variables. The defaults suit common Linux installations. Docker Desktop users can remove the `user` line if their file-sharing configuration requires the image's built-in user.
+`OND_UID` and `OND_GID` are required Compose interpolation values supplied by the command above; they are not application environment variables. Docker Desktop users can remove the `user` line if their file-sharing configuration requires the image's built-in user.
 
 ### Using a Docker Named Volume
 

docker-compose.yml

@@ -4,7 +4,7 @@ services:
       context: .
       dockerfile: Dockerfile
     image: open-network-diagram:local
-    user: '${OND_UID:-1000}:${OND_GID:-1000}'
+    user: '${OND_UID:?Set OND_UID to your host user ID}:${OND_GID:?Set OND_GID to your host group ID}'
     ports:
       - '8080:3000'
     volumes:

src/lib/components/NetworkDiagram.svelte

@@ -301,6 +301,9 @@
 	let subnetEditorOpen = false;
 	let showRackManager = false;
 
+	$: totalEntities = networkData.machines.length + networkData.devices.length;
+	$: emptyWritableDataset = writable && totalEntities === 0;
+
 	// Writable empty datasets use a session-only dismissal so onboarding
 	// returns after reload until the user adds a real entity. Sample-data
 	// dismissal remains persisted for returning demo users.
@@ -310,7 +313,7 @@
 	let emptyFirstRunDismissedThisSession = false;
 
 	function dismissFirstRun() {
-		if (writable && totalEntities === 0) {
+		if (emptyWritableDataset) {
 			emptyFirstRunDismissedThisSession = true;
 			return;
 		}
@@ -328,7 +331,6 @@
 		}
 	}
 
-	$: totalEntities = networkData.machines.length + networkData.devices.length;
 	$: allExampleNames =
 		totalEntities > 0 &&
 		[
@@ -338,7 +340,7 @@
 	$: showFirstRun =
 		hasLoadedInitialData &&
 		!isLoadingData &&
-		((writable && totalEntities === 0 && !emptyFirstRunDismissedThisSession) ||
+		((emptyWritableDataset && !emptyFirstRunDismissedThisSession) ||
 			(!firstRunDismissed && totalEntities <= 4 && allExampleNames));
 	let copiedIp: string | null = null;
 	let copiedIpTimer: ReturnType<typeof setTimeout> | null = null;
@@ -704,10 +706,12 @@
 	}
 
 	function resolveStaticFallbackNotice(reason: string | null): string {
-		if (!reason) {
+		const normalizedReason = reason?.trim();
+		if (!normalizedReason) {
 			return 'Read-only: API unavailable; using bundled static data.';
 		}
-		return `Read-only: ${reason} Using bundled static data.`;
+		const separator = /[.!?]$/.test(normalizedReason) ? ' ' : '. ';
+		return `Read-only: ${normalizedReason}${separator}Using bundled static data.`;
 	}
 
 	// Nodes are invisible hit targets (interaction, layout, edge anchoring);
@@ -2527,7 +2531,7 @@
 				isSampleData={allExampleNames}
 				{writable}
 				hasMachines={networkData.machines.length > 0}
-				storageReady={writable && totalEntities === 0}
+				storageReady={emptyWritableDataset}
 				on:addmachine={addMachine}
 				on:connectport={openFirstMachineEditor}
 				on:openipam={() => (showIpamPanel = true)}

src/lib/shared/networkPersistenceCore.mjs

@@ -19,7 +19,7 @@ import path from 'node:path';
  * @typedef {{ status: 'ready'; data: NetworkData; source: string; updatedAt: string } | { status: 'missing' | 'blank'; source: string; updatedAt: string | null }} NetworkFileReadState
  * @typedef {NetworkFileReadResult & { initialized: boolean }} NetworkFileBootstrapResult
  * @typedef {{ writable: boolean; reason: string | null }} WritableState
- * @typedef {{ createBackup?: boolean }} WriteNetworkFileOptions
+ * @typedef {{ createBackup?: boolean; beforeCommit?: () => Promise<void> }} WriteNetworkFileOptions
  */
 
 const NETWORK_READ_ONLY = process.env.NETWORK_READ_ONLY === 'true';
@@ -29,6 +29,12 @@ const EMPTY_NETWORK_DATA = Object.freeze({
 	machines: Object.freeze([]),
 	devices: Object.freeze([])
 });
+/** @type {Map<string, Promise<unknown>>} */
+const fileWriteOperations = new Map();
+/** @type {Map<string, Promise<NetworkFileBootstrapResult>>} */
+const initializationFlights = new Map();
+
+class InitializationSupersededError extends Error {}
 
 /**
  * @returns {string}
@@ -193,21 +199,18 @@ export async function trimBackups(backupDirectory, baseName, keep = 5) {
 
 /**
  * @param {NetworkData} data
+ * @param {string} source
  * @param {WriteNetworkFileOptions} [options]
  * @returns {Promise<NetworkFileMetadata>}
  */
-export async function writeNetworkFile(data, options) {
+async function writeNetworkFileUnlocked(data, source, options) {
 	if (!isWriteEnabled()) {
 		throw new Error('Writes disabled by NETWORK_READ_ONLY=true.');
 	}
 
-	const source = resolveDataFilePath();
 	const backupDirectory = resolveBackupDirectory();
 	const directory = path.dirname(source);
 	await mkdir(directory, { recursive: true });
-	if (options?.createBackup ?? true) {
-		await createBackupIfPresent(source, backupDirectory);
-	}
 
 	const temporaryPath = `${source}.tmp-${Date.now()}-${process.pid}`;
 	const jsonPayload = `${JSON.stringify(data, null, '\t')}\n`;
@@ -219,10 +222,14 @@ export async function writeNetworkFile(data, options) {
 		await fileHandle.close();
 	}
 	try {
+		await options?.beforeCommit?.();
+		if (options?.createBackup ?? true) {
+			await createBackupIfPresent(source, backupDirectory);
+		}
 		await rename(temporaryPath, source);
-	} catch (renameError) {
+	} catch (error) {
 		await unlink(temporaryPath).catch(() => undefined);
-		throw renameError;
+		throw error;
 	}
 
 	await trimBackups(backupDirectory, path.basename(source), 5);
@@ -234,6 +241,36 @@ export async function writeNetworkFile(data, options) {
 	};
 }
 
+/**
+ * @template T
+ * @param {string} source
+ * @param {() => Promise<T>} operation
+ * @returns {Promise<T>}
+ */
+async function withFileWriteLock(source, operation) {
+	const previousOperation = fileWriteOperations.get(source) ?? Promise.resolve();
+	const currentOperation = previousOperation.catch(() => undefined).then(operation);
+	fileWriteOperations.set(source, currentOperation);
+
+	try {
+		return await currentOperation;
+	} finally {
+		if (fileWriteOperations.get(source) === currentOperation) {
+			fileWriteOperations.delete(source);
+		}
+	}
+}
+
+/**
+ * @param {NetworkData} data
+ * @param {WriteNetworkFileOptions} [options]
+ * @returns {Promise<NetworkFileMetadata>}
+ */
+export function writeNetworkFile(data, options) {
+	const source = resolveDataFilePath();
+	return withFileWriteLock(source, () => writeNetworkFileUnlocked(data, source, options));
+}
+
 /**
  * @returns {Promise<WritableState>}
  */
@@ -302,7 +339,7 @@ export async function checkWritableState() {
  *
  * @returns {Promise<NetworkFileBootstrapResult>}
  */
-export async function readOrInitializeNetworkFile() {
+async function initializeNetworkFile() {
 	const state = await readNetworkFileState();
 	if (state.status === 'ready') {
 		return {
@@ -323,7 +360,34 @@ export async function readOrInitializeNetworkFile() {
 		machines: [...EMPTY_NETWORK_DATA.machines],
 		devices: [...EMPTY_NETWORK_DATA.devices]
 	};
-	const metadata = await writeNetworkFile(data, { createBackup: false });
+	let metadata;
+	try {
+		metadata = await withFileWriteLock(state.source, () =>
+			writeNetworkFileUnlocked(data, state.source, {
+				createBackup: false,
+				beforeCommit: async () => {
+					const currentState = await readNetworkFileState();
+					if (currentState.status === 'ready') {
+						throw new InitializationSupersededError();
+					}
+				}
+			})
+		);
+	} catch (error) {
+		if (!(error instanceof InitializationSupersededError)) {
+			throw error;
+		}
+		const currentState = await readNetworkFileState();
+		if (currentState.status !== 'ready') {
+			throw new Error(`Network data file "${currentState.source}" is ${currentState.status}.`);
+		}
+		return {
+			data: currentState.data,
+			source: currentState.source,
+			updatedAt: currentState.updatedAt,
+			initialized: false
+		};
+	}
 	return {
 		data,
 		source: metadata.source,
@@ -331,3 +395,24 @@ export async function readOrInitializeNetworkFile() {
 		initialized: true
 	};
 }
+
+/**
+ * @returns {Promise<NetworkFileBootstrapResult>}
+ */
+export function readOrInitializeNetworkFile() {
+	const source = resolveDataFilePath();
+	const existingFlight = initializationFlights.get(source);
+	if (existingFlight) {
+		return existingFlight;
+	}
+
+	const flight = initializeNetworkFile();
+	initializationFlights.set(source, flight);
+	const clearFlight = () => {
+		if (initializationFlights.get(source) === flight) {
+			initializationFlights.delete(source);
+		}
+	};
+	flight.then(clearFlight, clearFlight);
+	return flight;
+}

tests/networkPersistenceCore.test.mjs

@@ -89,6 +89,23 @@ test('does not rewrite an existing valid empty network', async () => {
 	});
 });
 
+test('does not overwrite real data written while a missing file is being initialized', async () => {
+	await withPersistenceEnvironment(async ({ persistence, source }) => {
+		const realData = {
+			machines: [{ machineName: 'Router' }],
+			devices: []
+		};
+
+		const bootstrap = persistence.readOrInitializeNetworkFile();
+		const write = persistence.writeNetworkFile(realData, { createBackup: false });
+		const [bootstrapResult] = await Promise.all([bootstrap, write]);
+
+		assert.equal(bootstrapResult.initialized, false);
+		assert.deepEqual(bootstrapResult.data, realData);
+		assert.deepEqual(JSON.parse(await readFile(source, 'utf8')), realData);
+	});
+});
+
 test('does not replace malformed or schema-invalid non-empty JSON', async () => {
 	await withPersistenceEnvironment(async ({ persistence, source }) => {
 		await persistence.writeNetworkFile({ machines: [], devices: [] }, { createBackup: false });