fix(widgets): deleting a duplicated widget also deleted the original

src/context/free-widget.context.tsx

@@ -31,6 +31,12 @@ import {
 	type WidgetPosition,
 	type WidgetSize,
 } from '@/layouts/widgets/layout-engine/types'
+import {
+	applyInstanceIdMap,
+	buildInstanceIdMap,
+	dedupeInstanceIds,
+	isServerInstanceId,
+} from '@/layouts/widgets/instance-id'
 import { migrateWidgetLayoutIfNeeded } from '@/layouts/widgets/migration'
 import { WIDGET_DEFINITIONS } from '@/layouts/widgets/widget-registry'
 import {
@@ -123,7 +129,7 @@ function normalizeWidgetSizes(layout: StoredWidget[], cols: number): StoredWidge
 }
 
 function sanitizeLayout(layout: StoredWidget[], cols: number): StoredWidget[] {
-	const sized = normalizeWidgetSizes(layout, cols)
+	const sized = normalizeWidgetSizes(dedupeInstanceIds(layout), cols)
 
 	if (validateLayout(sized, cols)) {
 		return sized
@@ -381,25 +387,10 @@ export function FreeWidgetProvider({ children }: { children: React.ReactNode })
 
 						if (synced && synced.length > 0) {
 							setSavedLayout((prev) => {
-								const updated = prev.map((w, index) => {
-									const matching =
-										synced.find(
-											(s) => s.instanceId === w.instanceId
-										) ||
-										synced.find((s) => s.widgetKey === w.id) ||
-										synced[index]
-									if (
-										matching?.instanceId &&
-										matching.instanceId !== w.instanceId
-									) {
-										return {
-											...w,
-											instanceId: matching.instanceId,
-											widgetId: matching.instanceId,
-										}
-									}
-									return w
-								})
+								const idMap = buildInstanceIdMap(prev, synced)
+								if (idMap.size === 0) return prev
+
+								const updated = applyInstanceIdMap(prev, idMap)
 								savedLayoutRef.current = updated
 								persistLayout(updated)
 								return updated
@@ -445,42 +436,16 @@ export function FreeWidgetProvider({ children }: { children: React.ReactNode })
 					.then((synced) => {
 						if (!synced || synced.length === 0) return
 
-						const idMap = new Map<string, string>()
-						currentLayout.forEach((w, index) => {
-							const isValidId =
-								typeof w.instanceId === 'string' &&
-								/^[0-9a-fA-F]{24}$/.test(w.instanceId)
-							if (isValidId) return
-							const matching =
-								synced.find((s) => s.widgetKey === w.id) || synced[index]
-							if (
-								matching?.instanceId &&
-								matching.instanceId !== w.instanceId
-							) {
-								idMap.set(w.instanceId, matching.instanceId)
-							}
-						})
-
+						const idMap = buildInstanceIdMap(currentLayout, synced)
 						if (idMap.size === 0) return
 
-						const applyIdMap = (list: StoredWidget[]) =>
-							list.map((w) =>
-								idMap.has(w.instanceId)
-									? {
-											...w,
-											instanceId: idMap.get(w.instanceId) as string,
-											widgetId: idMap.get(w.instanceId) as string,
-										}
-									: w
-							)
-
 						setSavedLayout((prev) => {
-							const updated = applyIdMap(prev)
+							const updated = applyInstanceIdMap(prev, idMap)
 							savedLayoutRef.current = updated
 							persistLayout(updated)
 							return updated
 						})
-						applyRuntimeLayout((prev) => applyIdMap(prev))
+						applyRuntimeLayout((prev) => applyInstanceIdMap(prev, idMap))
 					})
 					.catch(() => {})
 			}, 1000)
@@ -899,7 +864,7 @@ export function FreeWidgetProvider({ children }: { children: React.ReactNode })
 				setSelectedInstanceId(null)
 			}
 
-			if (isAuthenticated && typeof instanceId === 'string' && instanceId.trim()) {
+			if (isAuthenticated && isServerInstanceId(instanceId)) {
 				deleteUserWidgetApi(instanceId).catch(() => {})
 			}
 

src/layouts/widgets/__tests__/instance-id.test.ts

@@ -0,0 +1,193 @@
+import { describe, expect, it } from 'bun:test'
+import {
+	applyInstanceIdMap,
+	buildInstanceIdMap,
+	dedupeInstanceIds,
+	isServerInstanceId,
+} from '../instance-id'
+import { resolveLayoutChange } from '../layout-engine'
+import type { StoredWidget } from '../layout-engine/types'
+import { WidgetKeys } from '../layout-engine/types'
+
+function widget(
+	instanceId: string,
+	id: WidgetKeys = WidgetKeys.clock,
+	col = 0,
+	row = 0
+): StoredWidget {
+	return { id, instanceId, position: { col, row }, size: { w: 2, h: 1 } }
+}
+
+const SERVER_A = 'aaaaaaaaaaaaaaaaaaaaaaaa'
+const SERVER_B = 'bbbbbbbbbbbbbbbbbbbbbbbb'
+
+describe('isServerInstanceId', () => {
+	it('accepts only 24-char hex ids', () => {
+		expect(isServerInstanceId(SERVER_A)).toBe(true)
+		expect(isServerInstanceId('clock-abc123')).toBe(false)
+		expect(isServerInstanceId('')).toBe(false)
+		expect(isServerInstanceId(undefined)).toBe(false)
+	})
+})
+
+describe('buildInstanceIdMap', () => {
+	it('never assigns a server id already held by another widget', () => {
+		const layout = [
+			widget(SERVER_A, WidgetKeys.clock),
+			widget('clock-local', WidgetKeys.clock),
+		]
+		const synced = [
+			{ instanceId: SERVER_A, widgetKey: WidgetKeys.clock },
+			{ instanceId: SERVER_B, widgetKey: WidgetKeys.clock },
+		]
+
+		const idMap = buildInstanceIdMap(layout, synced)
+
+		expect(idMap.get('clock-local')).toBe(SERVER_B)
+
+		const updated = applyInstanceIdMap(layout, idMap)
+		const ids = updated.map((w) => w.instanceId)
+		expect(new Set(ids).size).toBe(ids.length)
+	})
+
+	it('does not map two local widgets onto the same server id', () => {
+		const layout = [widget('clock-1', WidgetKeys.clock), widget('clock-2', WidgetKeys.clock)]
+		const synced = [{ instanceId: SERVER_A, widgetKey: WidgetKeys.clock }]
+
+		const idMap = buildInstanceIdMap(layout, synced)
+
+		expect(idMap.size).toBe(1)
+		const updated = applyInstanceIdMap(layout, idMap)
+		const ids = updated.map((w) => w.instanceId)
+		expect(new Set(ids).size).toBe(ids.length)
+	})
+
+	it('leaves widgets untouched when there is nothing to map', () => {
+		const layout = [widget(SERVER_A), widget(SERVER_B)]
+		const synced = [
+			{ instanceId: SERVER_A, widgetKey: WidgetKeys.clock },
+			{ instanceId: SERVER_B, widgetKey: WidgetKeys.clock },
+		]
+
+		expect(buildInstanceIdMap(layout, synced).size).toBe(0)
+		expect(applyInstanceIdMap(layout, new Map())).toBe(layout)
+	})
+
+	it('does not borrow a server id belonging to a different widget type', () => {
+		const layout = [widget('search-local', WidgetKeys.search)]
+		const synced = [{ instanceId: SERVER_A, widgetKey: WidgetKeys.clock }]
+
+		expect(buildInstanceIdMap(layout, synced).size).toBe(0)
+	})
+})
+
+describe('dedupeInstanceIds', () => {
+	it('repairs a layout where a duplicate shares the original instance id', () => {
+		const layout = [
+			widget(SERVER_A, WidgetKeys.clock, 0, 0),
+			widget(SERVER_A, WidgetKeys.clock, 2, 0),
+		]
+
+		const repaired = dedupeInstanceIds(layout)
+		const ids = repaired.map((w) => w.instanceId)
+
+		expect(new Set(ids).size).toBe(2)
+		expect(repaired[0].instanceId).toBe(SERVER_A)
+		expect(repaired[1].instanceId).not.toBe(SERVER_A)
+		expect(repaired[1].widgetId).toBe(repaired[1].instanceId)
+	})
+
+	it('returns the same array when every id is unique', () => {
+		const layout = [widget(SERVER_A), widget(SERVER_B)]
+		expect(dedupeInstanceIds(layout)).toBe(layout)
+	})
+})
+
+describe('duplicate operation', () => {
+	it('keeps the server issued instance id instead of inventing one', () => {
+		const layout = [widget(SERVER_A, WidgetKeys.clock)]
+
+		const result = resolveLayoutChange({
+			layout,
+			operation: 'duplicate',
+			instanceId: SERVER_A,
+			newWidget: {
+				id: WidgetKeys.clock,
+				instanceId: SERVER_B,
+				widgetId: SERVER_B,
+				position: { col: 0, row: 0 },
+				size: { w: 2, h: 1 },
+			},
+			cols: 8,
+		})
+
+		expect(result).not.toBeNull()
+		expect(result!.map((w) => w.instanceId).sort()).toEqual(
+			[SERVER_A, SERVER_B].sort()
+		)
+	})
+
+	it('removing the duplicate leaves the original in place', () => {
+		const layout = [widget(SERVER_A, WidgetKeys.clock)]
+
+		const duplicated = resolveLayoutChange({
+			layout,
+			operation: 'duplicate',
+			instanceId: SERVER_A,
+			newWidget: {
+				id: WidgetKeys.clock,
+				instanceId: SERVER_B,
+				widgetId: SERVER_B,
+				position: { col: 0, row: 0 },
+				size: { w: 2, h: 1 },
+			},
+			cols: 8,
+		})!
+
+		const afterRemove = resolveLayoutChange({
+			layout: duplicated,
+			operation: 'remove',
+			instanceId: SERVER_B,
+			cols: 8,
+		})
+
+		expect(afterRemove).not.toBeNull()
+		expect(afterRemove!.length).toBe(1)
+		expect(afterRemove![0].instanceId).toBe(SERVER_A)
+	})
+
+	it('rejects a duplicate whose instance id is already taken', () => {
+		const layout = [widget(SERVER_A, WidgetKeys.clock)]
+
+		const result = resolveLayoutChange({
+			layout,
+			operation: 'duplicate',
+			instanceId: SERVER_A,
+			newWidget: {
+				id: WidgetKeys.clock,
+				instanceId: SERVER_A,
+				widgetId: SERVER_A,
+				position: { col: 0, row: 0 },
+				size: { w: 2, h: 1 },
+			},
+			cols: 8,
+		})
+
+		expect(result).toBeNull()
+	})
+
+	it('still generates an id when no widget is supplied', () => {
+		const layout = [widget(SERVER_A, WidgetKeys.clock)]
+
+		const result = resolveLayoutChange({
+			layout,
+			operation: 'duplicate',
+			instanceId: SERVER_A,
+			cols: 8,
+		})
+
+		expect(result).not.toBeNull()
+		expect(result!.length).toBe(2)
+		expect(result![1].instanceId).not.toBe(SERVER_A)
+	})
+})

src/layouts/widgets/instance-id.ts

@@ -0,0 +1,97 @@
+import type { StoredWidget } from './layout-engine/types'
+
+const SERVER_INSTANCE_ID_PATTERN = /^[0-9a-fA-F]{24}$/
+
+export interface SyncedWidgetIdentity {
+	instanceId: string
+	widgetKey: string
+}
+
+export function isServerInstanceId(value: unknown): value is string {
+	return typeof value === 'string' && SERVER_INSTANCE_ID_PATTERN.test(value)
+}
+
+export function createLocalInstanceId(widgetKey: string): string {
+	return `${widgetKey}-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 8)}`
+}
+
+export function dedupeInstanceIds(layout: StoredWidget[]): StoredWidget[] {
+	const seen = new Set<string>()
+	let changed = false
+
+	const result = layout.map((widget) => {
+		if (widget.instanceId && !seen.has(widget.instanceId)) {
+			seen.add(widget.instanceId)
+			return widget
+		}
+
+		let nextId = createLocalInstanceId(widget.id)
+		while (seen.has(nextId)) {
+			nextId = createLocalInstanceId(widget.id)
+		}
+		seen.add(nextId)
+		changed = true
+
+		return { ...widget, instanceId: nextId, widgetId: nextId }
+	})
+
+	return changed ? result : layout
+}
+
+export function buildInstanceIdMap(
+	layout: StoredWidget[],
+	synced: SyncedWidgetIdentity[]
+): Map<string, string> {
+	const idMap = new Map<string, string>()
+	const claimed = new Set<string>()
+
+	for (const widget of layout) {
+		if (isServerInstanceId(widget.instanceId)) {
+			claimed.add(widget.instanceId)
+		}
+	}
+
+	for (const entry of synced) {
+		if (!entry?.instanceId) continue
+		const owner = layout.find((w) => w.instanceId === entry.instanceId)
+		if (owner) {
+			claimed.add(entry.instanceId)
+		}
+	}
+
+	layout.forEach((widget, index) => {
+		if (isServerInstanceId(widget.instanceId)) return
+
+		const candidates: SyncedWidgetIdentity[] = []
+		const byIndex = synced[index]
+		if (byIndex?.instanceId && byIndex.widgetKey === widget.id) {
+			candidates.push(byIndex)
+		}
+		for (const entry of synced) {
+			if (entry?.instanceId && entry.widgetKey === widget.id) {
+				candidates.push(entry)
+			}
+		}
+
+		const matching = candidates.find((entry) => !claimed.has(entry.instanceId))
+		if (!matching) return
+
+		claimed.add(matching.instanceId)
+		idMap.set(widget.instanceId, matching.instanceId)
+	})
+
+	return idMap
+}
+
+export function applyInstanceIdMap(
+	layout: StoredWidget[],
+	idMap: Map<string, string>
+): StoredWidget[] {
+	if (idMap.size === 0) return layout
+
+	return layout.map((widget) => {
+		const nextId = idMap.get(widget.instanceId)
+		if (!nextId) return widget
+		return { ...widget, instanceId: nextId, widgetId: nextId }
+	})
+}