fix(pptx): serialize automation commands, and enforce require-atomic-updates

ARCHITECTURE.md

@@ -58,6 +58,7 @@ slidecue/
 │   │   ├── index.ts             # Entry point, IPC handlers
 │   │   ├── pptx/
 │   │   │   ├── slideParser.ts   # PPTX file parser (JSZip)
+│   │   │   ├── serialize.ts     # One-command-at-a-time wrapper for a backend
 │   │   │   ├── macos.ts         # AppleScript automation
 │   │   │   └── windows.ts       # COM automation (TODO)
 │   │   └── server/
@@ -134,6 +135,17 @@ Extracts metadata from PowerPoint files using JSZip:
 - Speaker notes per slide (via relationship files)
 - Slide dimensions
 
+#### `pptx/serialize.ts` — Command Serialization
+
+Every backend caches the slideshow position in module-level state and refreshes
+it from PowerPoint across an `await`. `serializeAutomation()` chains all calls
+onto a single promise so only one is ever in flight, which is what keeps that
+cache in step with PowerPoint when several remotes, a double-tap, and the 500 ms
+`getSlideInfo()` poll all arrive at once. Each backend exports its automation
+object already wrapped, so both `main/index.ts` and `server/socket.ts` share one
+queue. Backend methods must therefore never call each other through the exported
+object.
+
 #### `pptx/macos.ts` — macOS Automation
 Controls PowerPoint via AppleScript:
 - `checkInstalled()` — Verify PowerPoint is installed

eslint.config.mjs

@@ -62,15 +62,10 @@ export default [
       'prefer-const': 'error',
       'no-var': 'error',
 
-      // 'warn', not 'error'. This flags seven pre-existing sites in the
-      // PowerPoint automation backends where module-level state (currentSlide,
-      // currentAnimationStep, localPresentationCopy) is read before an `await`
-      // and written after it. They read as genuine interleaving hazards under
-      // rapid remote input rather than false positives, but fixing them means
-      // reworking the concurrency model of code that can only be exercised
-      // against a real PowerPoint install - well outside the scope of turning
-      // linting on. Left visible rather than disabled so the debt stays counted.
-      'require-atomic-updates': 'warn',
+      // Not in `recommended`, kept anyway: it catches shared state that is read
+      // before an `await` and written after it, which is exactly the shape the
+      // PowerPoint backends keep their slideshow position in.
+      'require-atomic-updates': 'error',
 
       // Off on purpose, per typescript-eslint's own guidance: the compiler
       // already resolves every identifier, and keeping it on would mean

src/main/pptx/macos.ts

@@ -6,6 +6,7 @@ import { readdir, access, mkdir, unlink, copyFile } from 'fs/promises';
 import { join, basename } from 'path';
 import { tmpdir } from 'os';
 import { PowerPointAutomation, SlideInfo, SlideMetadata, ProgressCallback } from './types';
+import { serializeAutomation } from './serialize';
 import { parsePresentationData, PresentationData, getNextVisibleSlide, getSlideData } from './slideParser';
 import log from 'electron-log';
 
@@ -57,7 +58,7 @@ end tell
   }
 }
 
-export const macOSAutomation: PowerPointAutomation = {
+const macOSBackend: PowerPointAutomation = {
   async checkInstalled() {
     try {
       await access('/Applications/Microsoft PowerPoint.app');
@@ -242,7 +243,8 @@ end tell
   },
 
   async nextSlide() {
-    const slideData = presentationData ? getSlideData(currentSlide, presentationData) : null;
+    const slideBefore = currentSlide;
+    const slideData = presentationData ? getSlideData(slideBefore, presentationData) : null;
     const animationsOnSlide = slideData?.animationClicks || 0;
 
     // Send the keystroke
@@ -251,19 +253,27 @@ end tell
     // Wait for PowerPoint to process
     await new Promise(resolve => setTimeout(resolve, 100));
 
-    // Check if we advanced an animation or moved to next slide
-    if (currentAnimationStep < animationsOnSlide) {
-      // Might be an animation click
-      currentAnimationStep++;
-    }
-
     // Query PowerPoint for actual slide number
     const actualSlide = await queryCurrentSlide();
 
-    if (actualSlide !== currentSlide) {
-      // We moved to a new slide
-      currentSlide = actualSlide;
-      currentAnimationStep = 0;
+    // Read the state back after the await instead of reusing the snapshot taken
+    // above, then write both fields in one synchronous block. The serializer
+    // already stops another command from landing in between; the re-read is
+    // what makes that checkable rather than assumed.
+    const slideChanged = actualSlide !== currentSlide;
+    let newStep = currentAnimationStep;
+
+    // Either we advanced an animation or we moved to a new slide.
+    if (slideChanged) {
+      newStep = 0;
+    } else if (newStep < animationsOnSlide) {
+      newStep++;
+    }
+
+    currentSlide = actualSlide;
+    currentAnimationStep = newStep;
+
+    if (slideChanged) {
       console.log(`Moved to slide ${currentSlide}`);
     } else {
       console.log(`Animation ${currentAnimationStep}/${animationsOnSlide} on slide ${currentSlide}`);
@@ -380,14 +390,16 @@ tell application "Microsoft PowerPoint"
 end tell
     `);
 
-    // Clean up temp copy
-    if (localPresentationCopy) {
+    // Clean up temp copy. The path is cleared before the unlink is awaited, so
+    // the field is never left pointing at a file that is on its way out.
+    const tempCopy = localPresentationCopy;
+    if (tempCopy) {
+      localPresentationCopy = '';
       try {
-        await unlink(localPresentationCopy);
+        await unlink(tempCopy);
       } catch {
         // Ignore
       }
-      localPresentationCopy = '';
     }
 
     // Reset state
@@ -396,3 +408,7 @@ end tell
     currentAnimationStep = 0;
   },
 };
+
+// One command at a time: the module-level state above is only consistent if
+// nothing interleaves with it. See serialize.ts.
+export const macOSAutomation = serializeAutomation(macOSBackend);

src/main/pptx/serialize.ts

@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: 2025-2026 Schuberg Philis / Lab271
+import { PowerPointAutomation } from './types';
+
+/**
+ * Wraps a PowerPoint backend so that at most one automation call is ever in
+ * flight against it.
+ *
+ * Every backend keeps its slideshow position in module-level state
+ * (`currentSlide`, `currentAnimationStep`, ...) and refreshes it from
+ * PowerPoint across an `await` - an AppleScript round trip on macOS, a
+ * PowerShell bridge command or a COM call on Windows. Nothing used to stop two
+ * of those from overlapping: the web remote can fire `next` from several
+ * devices at once, a fast double-tap queues two `nextSlide()` calls, and the
+ * 500 ms poll in `server/socket.ts` calls `getSlideInfo()` on top of both. The
+ * second caller would then read state that the first had not finished writing
+ * and leave the cached position disagreeing with the real one.
+ *
+ * Chaining the calls onto a single promise fixes that at the source: the
+ * commands reach PowerPoint in the order they were requested, and each one sees
+ * the state the previous one committed. It also means a backend never has more
+ * than one outstanding request to its transport, which is what the PowerShell
+ * bridge's FIFO response matching already assumed.
+ *
+ * A rejected call does not poison the chain - the next queued call still runs,
+ * and the rejection is delivered only to the caller that asked for it.
+ */
+export function serializeAutomation(backend: PowerPointAutomation): PowerPointAutomation {
+  // Always settles fulfilled, so a failed call cannot stall everything behind it.
+  let tail: Promise<void> = Promise.resolve();
+
+  function run<T>(operation: () => Promise<T>): Promise<T> {
+    const result = tail.then(operation);
+    tail = result.then(
+      () => undefined,
+      () => undefined
+    );
+    return result;
+  }
+
+  // Listed one by one rather than proxied so that adding a method to
+  // PowerPointAutomation without serializing it is a type error.
+  return {
+    checkInstalled: () => run(() => backend.checkInstalled()),
+    openPresentation: (filePath) => run(() => backend.openPresentation(filePath)),
+    exportThumbnails: (outputDir, onProgress) =>
+      run(() => backend.exportThumbnails(outputDir, onProgress)),
+    startSlideshow: () => run(() => backend.startSlideshow()),
+    nextSlide: () => run(() => backend.nextSlide()),
+    prevSlide: () => run(() => backend.prevSlide()),
+    gotoSlide: (index) => run(() => backend.gotoSlide(index)),
+    getSlideInfo: () => run(() => backend.getSlideInfo()),
+    stopSlideshow: () => run(() => backend.stopSlideshow()),
+    closePresentation: () => run(() => backend.closePresentation()),
+  };
+}

src/main/pptx/windows-powershell.ts

@@ -8,6 +8,7 @@
 import log from 'electron-log';
 import { app } from 'electron';
 import { PowerPointAutomation, SlideInfo, SlideMetadata, ProgressCallback } from './types';
+import { serializeAutomation } from './serialize';
 import { PresentationData, getNextVisibleSlide, getSlideData } from './slideParser';
 
 interface PSCommand {
@@ -214,7 +215,7 @@
 let totalSlides = 1;
 let localPresentationCopy = '';
 
-export const windowsAutomation: PowerPointAutomation = {
+const powerShellBackend: PowerPointAutomation = {
   async checkInstalled() {
     try {
       log.info('[Windows] Checking if PowerPoint is installed');
@@ -265,7 +266,7 @@
         totalSlides = metadata.totalSlides;
 
         // Build presentationData from PowerPoint metadata
         const slides = metadata.slides.map((s: any) => ({
           slideNumber: s.slideNumber,
           name: `Slide ${s.slideNumber}`,
           hidden: s.hidden,
@@ -273,8 +274,8 @@
           notes: s.notes || ''
         }));
 
         const visibleSlides = slides.filter((s: any) => !s.hidden).map((s: any) => s.slideNumber);
         const hiddenSlides = slides.filter((s: any) => s.hidden).map((s: any) => s.slideNumber);
 
         presentationData = {
           slides,
@@ -378,10 +379,11 @@
 
   async nextSlide() {
     try {
-      const slideData = presentationData ? getSlideData(currentSlide, presentationData) : null;
+      const slideBefore = currentSlide;
+      const slideData = presentationData ? getSlideData(slideBefore, presentationData) : null;
       const animationsOnSlide = slideData?.animationClicks || 0;
 
-      log.info('[Windows] Next slide (current:', currentSlide, 'animation:', currentAnimationStep, '/', animationsOnSlide, ')');
+      log.info('[Windows] Next slide (current:', slideBefore, 'animation:', currentAnimationStep, '/', animationsOnSlide, ')');
 
       const response = await bridge.sendCommand({ action: 'next' });
 
@@ -390,15 +392,27 @@
         return;
       }
 
+      // Read the state back after the await instead of reusing the snapshot
+      // taken above, then write both fields in one synchronous block. The
+      // serializer already stops another command from landing in between; the
+      // re-read is what makes that checkable rather than assumed.
       const newSlide = parseInt(response.data || String(currentSlide), 10);
+      const slideChanged = newSlide !== currentSlide;
+      let newStep = currentAnimationStep;
+
+      // Either we advanced an animation or we moved to the next slide.
+      if (slideChanged) {
+        newStep = 0;
+      } else if (newStep < animationsOnSlide) {
+        newStep++;
+      }
+
+      currentSlide = newSlide;
+      currentAnimationStep = newStep;
 
-      // Check if we advanced an animation or moved to next slide
-      if (newSlide !== currentSlide) {
-        currentSlide = newSlide;
-        currentAnimationStep = 0;
+      if (slideChanged) {
         log.info('[Windows] Moved to slide', currentSlide);
-      } else if (currentAnimationStep < animationsOnSlide) {
-        currentAnimationStep++;
+      } else {
         log.info('[Windows] Animation', currentAnimationStep, '/', animationsOnSlide, 'on slide', currentSlide);
       }
     } catch (error) {
@@ -417,7 +431,11 @@
         return;
       }
 
-      currentSlide = parseInt(response.data || String(currentSlide), 10);
+      // Re-read the state after the await rather than reusing the value logged
+      // above, so the assignment cannot be based on the pre-command position.
+      const slideBefore = currentSlide;
+      const newSlide = parseInt(response.data || String(slideBefore), 10);
+      currentSlide = newSlide;
       currentAnimationStep = 0;
       log.info('[Windows] Moved to slide', currentSlide);
     } catch (error) {
@@ -523,15 +541,17 @@
       // Stop the bridge
       await bridge.stop();
 
-      // Clean up temp copy
-      if (localPresentationCopy) {
+      // Clean up temp copy. The path is cleared before the unlink is awaited,
+      // so the field is never left pointing at a file that is on its way out.
+      const tempCopy = localPresentationCopy;
+      if (tempCopy) {
+        localPresentationCopy = '';
         try {
-          await unlink(localPresentationCopy);
+          await unlink(tempCopy);
           log.info('[Windows] Deleted temp presentation copy');
         } catch (error) {
           log.error('[Windows] Failed to delete temp copy:', error);
         }
-        localPresentationCopy = '';
       }
 
       // Reset state
@@ -544,3 +564,7 @@
     }
   },
 };
+
+// One command at a time: the module-level state above is only consistent if
+// nothing interleaves with it. See serialize.ts.
+export const windowsAutomation = serializeAutomation(powerShellBackend);

src/main/pptx/windows-winax.ts

@@ -5,10 +5,11 @@
 import { tmpdir } from 'os';
 import log from 'electron-log';
 import { PowerPointAutomation, SlideInfo, SlideMetadata, ProgressCallback } from './types';
+import { serializeAutomation } from './serialize';
 import { parsePresentationData, PresentationData, getNextVisibleSlide, getSlideData } from './slideParser';
 
 // Try to load winax
 let winax: any = null;
 try {
   winax = require('winax');
   log.info('[Windows-WinAX] Loaded winax module successfully');
@@ -17,9 +18,9 @@
 }
 
 // Presentation state
 let pptApp: any = null;
 let presentation: any = null;
 let slideShow: any = null;
 let presentationData: PresentationData | null = null;
 let currentSlide = 1;
 let currentAnimationStep = 0;
@@ -40,7 +41,7 @@
   return currentSlide;
 }
 
-export const windowsWinaxAutomation: PowerPointAutomation = {
+const winaxBackend: PowerPointAutomation = {
   async checkInstalled() {
     if (!winax) {
       log.error('[Windows-WinAX] winax module not available');
@@ -215,9 +216,12 @@
 
       await new Promise(resolve => setTimeout(resolve, 100));
 
-      currentSlide = queryCurrentSlide();
+      // Logging the move re-reads the state after the await, so the assignment
+      // below cannot be based on the value captured before the COM call.
+      const actualSlide = queryCurrentSlide();
+      log.info('[Windows-WinAX] Moved from slide', currentSlide, 'to slide', actualSlide);
+      currentSlide = actualSlide;
       currentAnimationStep = 0;
-      log.info('[Windows-WinAX] Moved to slide', currentSlide);
     } catch (error) {
       log.error('[Windows-WinAX] Failed to go to previous slide:', error);
     }
@@ -333,15 +337,17 @@
         pptApp = null;
       }
 
-      // Clean up temp copy
-      if (localPresentationCopy) {
+      // Clean up temp copy. The path is cleared before the unlink is awaited,
+      // so the field is never left pointing at a file that is on its way out.
+      const tempCopy = localPresentationCopy;
+      if (tempCopy) {
+        localPresentationCopy = '';
         try {
-          await unlink(localPresentationCopy);
+          await unlink(tempCopy);
           log.info('[Windows-WinAX] Deleted temp presentation copy');
         } catch (error) {
           log.error('[Windows-WinAX] Failed to delete temp copy:', error);
         }
-        localPresentationCopy = '';
       }
 
       // Reset state
@@ -355,3 +361,7 @@
     }
   },
 };
+
+// One command at a time: the module-level state above is only consistent if
+// nothing interleaves with it. See serialize.ts.
+export const windowsWinaxAutomation = serializeAutomation(winaxBackend);