Address PR #40 review feedback on icon layout and drag interactions

Co-authored-by: mroxso <24775431+mroxso@users.noreply.github.com>

src/components/os/Desktop.tsx

@@ -152,7 +152,7 @@ export function Desktop() {
                   selected={selected === app.id}
                   dragging={dragging === app.id}
                   pickedUp={picked === app.id}
-                  tabIndex={selected === null || selected === app.id ? 0 : -1}
+                  tabIndex={0}
                   style={{ position: 'absolute', left: SURFACE_PADDING + displaySlot.col * CELL_WIDTH, top: SURFACE_PADDING + displaySlot.row * CELL_HEIGHT, zIndex: dragging === app.id ? 2 : 1 }}
                   onPointerDown={(event) => {
                     if (event.button !== 0) return;

src/components/os/MobileAppShell.tsx

@@ -164,6 +164,7 @@ function HomeScreen({ onOpen }: { onOpen: (id: string) => void }) {
   const [announcement, setAnnouncement] = useState('');
   const dragStart = useRef<{ id: string; x: number; y: number; moved: boolean } | null>(null);
   const suppressClick = useRef(false);
+  const targetRef = useRef<string | null>(null);
   const { layout, setMobile } = useIconLayout(apps.map((app) => app.id), { columns, rows: Math.max(8, Math.ceil(apps.length / columns) + 4) });
   const byId = useMemo(() => new Map(apps.map((app) => [app.id, app])), [apps]);
   const orderedApps = layout.mobile.map((id) => byId.get(id)).filter((app): app is NonNullable<typeof app> => Boolean(app));
@@ -194,16 +195,20 @@ function HomeScreen({ onOpen }: { onOpen: (id: string) => void }) {
       active.moved = true;
       setDragging(active.id);
       const hit = document.elementFromPoint(event.clientX, event.clientY)?.closest<HTMLElement>('[data-home-icon-id]');
-      setTarget(hit?.dataset.homeIconId ?? null);
+      const nextTarget = hit?.dataset.homeIconId ?? null;
+      targetRef.current = nextTarget;
+      setTarget(nextTarget);
     };
     const onEnd = () => {
       const active = dragStart.current;
       if (active?.moved) {
         suppressClick.current = true;
+        const target = targetRef.current;
         reorder(active.id, target && target !== active.id ? target : null);
       }
       dragStart.current = null;
       setDragging(null);
+      targetRef.current = null;
       setTarget(null);
     };
     window.addEventListener('pointermove', onMove);
@@ -214,7 +219,7 @@ function HomeScreen({ onOpen }: { onOpen: (id: string) => void }) {
       window.removeEventListener('pointerup', onEnd);
       window.removeEventListener('pointercancel', onEnd);
     };
-  }, [reorder, target]);
+  }, [reorder]);
 
   const onKeyDown = (id: string, event: React.KeyboardEvent<HTMLButtonElement>) => {
     const index = layout.mobile.indexOf(id);

src/os/iconLayout.test.ts

@@ -0,0 +1,129 @@
+import { describe, expect, it } from 'vitest';
+import { defaultDesktopLayout, reconcileDesktopLayout, reconcileMobileLayout, swapDesktopSlots } from './iconLayout';
+
+const geometry = { columns: 4, rows: 4 };
+
+describe('defaultDesktopLayout', () => {
+  it('places apps in row-major order starting from the first cell', () => {
+    const slots = defaultDesktopLayout(['a', 'b', 'c'], geometry);
+    expect(slots).toEqual([
+      { id: 'a', col: 0, row: 0 },
+      { id: 'b', col: 1, row: 0 },
+      { id: 'c', col: 2, row: 0 },
+    ]);
+  });
+
+  it('keeps overflow icons on the last row when there are more apps than cells', () => {
+    const ids = Array.from({ length: 18 }, (_, i) => `app-${i}`);
+    const slots = defaultDesktopLayout(ids, geometry);
+    for (const slot of slots) {
+      expect(slot.row).toBeLessThan(geometry.rows);
+    }
+  });
+});
+
+describe('reconcileDesktopLayout', () => {
+  it('drops unknown ids and keeps known ones in place', () => {
+    const saved = [
+      { id: 'a', col: 0, row: 0 },
+      { id: 'stale', col: 1, row: 0 },
+    ];
+    const reconciled = reconcileDesktopLayout(saved, ['a'], geometry);
+    expect(reconciled).toEqual([{ id: 'a', col: 0, row: 0 }]);
+  });
+
+  it('clamps out-of-bounds coordinates to the grid', () => {
+    const saved = [{ id: 'a', col: 99, row: 99 }];
+    const reconciled = reconcileDesktopLayout(saved, ['a'], geometry);
+    expect(reconciled[0].col).toBeLessThan(geometry.columns);
+    expect(reconciled[0].row).toBeLessThan(geometry.rows);
+  });
+
+  it('resolves collisions by moving the later slot to the next free cell', () => {
+    const saved = [
+      { id: 'a', col: 0, row: 0 },
+      { id: 'b', col: 0, row: 0 },
+    ];
+    const reconciled = reconcileDesktopLayout(saved, ['a', 'b'], geometry);
+    const a = reconciled.find((slot) => slot.id === 'a');
+    const b = reconciled.find((slot) => slot.id === 'b');
+    expect(a).toEqual({ id: 'a', col: 0, row: 0 });
+    expect(b).not.toEqual({ id: 'b', col: 0, row: 0 });
+  });
+
+  it('appends new apps to the first free cell after known ones', () => {
+    const saved = [{ id: 'a', col: 0, row: 0 }];
+    const reconciled = reconcileDesktopLayout(saved, ['a', 'b'], geometry);
+    expect(reconciled).toEqual([
+      { id: 'a', col: 0, row: 0 },
+      { id: 'b', col: 1, row: 0 },
+    ]);
+  });
+
+  it('drops duplicate saved entries for the same id', () => {
+    const saved = [
+      { id: 'a', col: 0, row: 0 },
+      { id: 'a', col: 1, row: 0 },
+    ];
+    const reconciled = reconcileDesktopLayout(saved, ['a'], geometry);
+    expect(reconciled).toEqual([{ id: 'a', col: 0, row: 0 }]);
+  });
+
+  it('keeps overflow icons within the grid when there are more apps than cells', () => {
+    const ids = Array.from({ length: 18 }, (_, i) => `app-${i}`);
+    const reconciled = reconcileDesktopLayout([], ids, geometry);
+    for (const slot of reconciled) {
+      expect(slot.row).toBeLessThan(geometry.rows);
+      expect(slot.col).toBeLessThan(geometry.columns);
+    }
+  });
+});
+
+describe('reconcileMobileLayout', () => {
+  it('keeps known ids in their saved order and drops unknown ones', () => {
+    const ordered = reconcileMobileLayout(['b', 'stale', 'a'], ['a', 'b']);
+    expect(ordered).toEqual(['b', 'a']);
+  });
+
+  it('appends new apps that were not in the saved order', () => {
+    const ordered = reconcileMobileLayout(['a'], ['a', 'b']);
+    expect(ordered).toEqual(['a', 'b']);
+  });
+
+  it('drops duplicate saved entries for the same id', () => {
+    const ordered = reconcileMobileLayout(['a', 'a', 'b'], ['a', 'b']);
+    expect(ordered).toEqual(['a', 'b']);
+  });
+});
+
+describe('swapDesktopSlots', () => {
+  it('swaps the moved icon with the icon occupying the target cell', () => {
+    const slots = [
+      { id: 'a', col: 0, row: 0 },
+      { id: 'b', col: 1, row: 0 },
+    ];
+    const next = swapDesktopSlots(slots, 'a', { col: 1, row: 0 });
+    expect(next).toEqual([
+      { id: 'a', col: 1, row: 0 },
+      { id: 'b', col: 0, row: 0 },
+    ]);
+  });
+
+  it('moves the icon to an empty cell without disturbing others', () => {
+    const slots = [
+      { id: 'a', col: 0, row: 0 },
+      { id: 'b', col: 1, row: 0 },
+    ];
+    const next = swapDesktopSlots(slots, 'a', { col: 2, row: 0 });
+    expect(next).toEqual([
+      { id: 'a', col: 2, row: 0 },
+      { id: 'b', col: 1, row: 0 },
+    ]);
+  });
+
+  it('returns the original slots unchanged when the id is not found', () => {
+    const slots = [{ id: 'a', col: 0, row: 0 }];
+    const next = swapDesktopSlots(slots, 'missing', { col: 1, row: 0 });
+    expect(next).toBe(slots);
+  });
+});

src/os/iconLayout.ts

@@ -38,8 +38,8 @@ function firstFree(occupied: Set<string>, geometry: GridGeometry): Omit<DesktopS
     }
   }
   // A very small viewport can temporarily have fewer cells than apps. Keep
-  // the remaining icons in the next stable row; rendering clamps on resize.
-  return { col: 0, row: geometry.rows };
+  // the remaining icons in the last stable row so they stay on-screen.
+  return { col: 0, row: Math.max(0, geometry.rows - 1) };
 }
 
 export function defaultDesktopLayout(ids: string[], geometry: GridGeometry): DesktopSlot[] {

src/os/useIconLayout.ts

@@ -27,10 +27,15 @@ export function useIconLayout(ids: string[], geometry: GridGeometry) {
     mobile: reconcileMobileLayout(layout.mobile, stableIds),
   }), [layout, stableGeometry, stableIds]);
 
+  const dirty = useRef(false);
   const saveTimer = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);
   useEffect(() => {
+    if (!dirty.current) return;
     clearTimeout(saveTimer.current);
-    saveTimer.current = setTimeout(() => saveIconLayout(normalized), SAVE_DELAY);
+    saveTimer.current = setTimeout(() => {
+      dirty.current = false;
+      saveIconLayout(normalized);
+    }, SAVE_DELAY);
     return () => clearTimeout(saveTimer.current);
   }, [normalized]);
 
@@ -49,12 +54,14 @@ export function useIconLayout(ids: string[], geometry: GridGeometry) {
   }, [stableGeometry, stableIds]);
 
   const setDesktop = useCallback((updater: (slots: DesktopSlot[]) => DesktopSlot[]) => {
+    dirty.current = true;
     setLayout((current) => ({
       ...current,
       desktop: updater(reconcileDesktopLayout(current.desktop, stableIds, stableGeometry)),
     }));
   }, [stableGeometry, stableIds]);
   const setMobile = useCallback((updater: (order: string[]) => string[]) => {
+    dirty.current = true;
     setLayout((current) => ({ ...current, mobile: updater(reconcileMobileLayout(current.mobile, stableIds)) }));
   }, [stableIds]);
   const reset = useCallback((profile: 'desktop' | 'mobile' | 'both') => {
@@ -63,6 +70,8 @@ export function useIconLayout(ids: string[], geometry: GridGeometry) {
       desktop: profile === 'mobile' ? normalized.desktop : defaultDesktopLayout(stableIds, stableGeometry),
       mobile: profile === 'desktop' ? normalized.mobile : [...stableIds],
     };
+    dirty.current = false;
+    clearTimeout(saveTimer.current);
     saveIconLayout(next);
     setLayout(next);
     window.dispatchEvent(new Event('icon-layout-reset'));