From 13aec26c17b8beea69c759d06906a67326c1d158 Mon Sep 17 00:00:00 2001 From: Shak Date: Sat, 29 Aug 2026 19:20:53 +0330 Subject: [PATCH] fix(widgets): stop a duplicated widget from taking over the original's instance id Deleting a duplicated widget sometimes removed the original one too. Three bugs chained together to cause it: 1. resolveLayoutChange('duplicate') ignored the newWidget argument and generated its own throwaway local id, so the instance id issued by createUserWidgetApi was discarded and the duplicate entered the layout with an id the server had never seen. 2. Because that id did not look like a server id, the next sync tried to remap it. The mapping picked its target with synced.find(s => s.widgetKey === w.id), which returns the first record for that widget key. With two instances of the same widget that is the original's record, so the duplicate was assigned the original's instance id and the layout ended up holding two widgets with identical ids. 3. remove filters by instance id, so removing either one dropped both, and deleteUserWidgetApi was called with the shared id. Fixes: - duplicate now uses the supplied newWidget, keeping the server issued instance id, and refuses an id that is already present in the layout. - Add buildInstanceIdMap, which prefers the index aligned record, requires a matching widgetKey, and never hands out an id that another widget already holds or that an earlier widget claimed in the same pass. Both sync paths use it. - Add dedupeInstanceIds to sanitizeLayout so layouts already corrupted by this bug are repaired on load: the first widget keeps the id and any later collision is reassigned a fresh one. - removeWidget only calls deleteUserWidgetApi for real server ids instead of any non-empty string. Covered by tests for the id mapping guards, the dedupe repair, and a duplicate-then-remove case asserting the original survives. --- src/context/free-widget.context.tsx | 65 ++---- .../widgets/__tests__/instance-id.test.ts | 193 ++++++++++++++++++ src/layouts/widgets/instance-id.ts | 97 +++++++++ .../widgets/layout-engine/layout-engine.ts | 21 +- 4 files changed, 320 insertions(+), 56 deletions(-) create mode 100644 src/layouts/widgets/__tests__/instance-id.test.ts create mode 100644 src/layouts/widgets/instance-id.ts diff --git a/src/context/free-widget.context.tsx b/src/context/free-widget.context.tsx index aa7c4eb7..be9a4f81 100644 --- a/src/context/free-widget.context.tsx +++ b/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() - 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(() => {}) } diff --git a/src/layouts/widgets/__tests__/instance-id.test.ts b/src/layouts/widgets/__tests__/instance-id.test.ts new file mode 100644 index 00000000..10d7c15b --- /dev/null +++ b/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) + }) +}) diff --git a/src/layouts/widgets/instance-id.ts b/src/layouts/widgets/instance-id.ts new file mode 100644 index 00000000..7f1cc5f1 --- /dev/null +++ b/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() + 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 { + const idMap = new Map() + const claimed = new Set() + + 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 +): 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 } + }) +} diff --git a/src/layouts/widgets/layout-engine/layout-engine.ts b/src/layouts/widgets/layout-engine/layout-engine.ts index bfabae6e..a90af7e3 100644 --- a/src/layouts/widgets/layout-engine/layout-engine.ts +++ b/src/layouts/widgets/layout-engine/layout-engine.ts @@ -153,12 +153,21 @@ export function resolveLayoutChange( const original = layout.find((w) => w.instanceId === instanceId) if (!original) return null - const newInstanceId = `${original.id}-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 6)}` - const duplicated: StoredWidget = { - id: original.id, - instanceId: newInstanceId, - size: { ...original.size }, - position: findAvailableSlot(layout, original.size, cols), + const duplicated: StoredWidget = newWidget + ? { + ...newWidget, + size: { ...newWidget.size }, + position: findAvailableSlot(layout, newWidget.size, cols), + } + : { + id: original.id, + instanceId: `${original.id}-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 6)}`, + size: { ...original.size }, + position: findAvailableSlot(layout, original.size, cols), + } + + if (layout.some((w) => w.instanceId === duplicated.instanceId)) { + return null } const appended = [...layout, duplicated]