diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 328dc3d..9b41c3e 100644 --- a/ARCHITECTURE.md +++ b/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 diff --git a/eslint.config.mjs b/eslint.config.mjs index ca76829..bddf108 100644 --- a/eslint.config.mjs +++ b/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 diff --git a/src/main/pptx/macos.ts b/src/main/pptx/macos.ts index fa87dbe..0d941cf 100644 --- a/src/main/pptx/macos.ts +++ b/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); diff --git a/src/main/pptx/serialize.ts b/src/main/pptx/serialize.ts new file mode 100644 index 0000000..66aa168 --- /dev/null +++ b/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 = Promise.resolve(); + + function run(operation: () => Promise): Promise { + 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()), + }; +} diff --git a/src/main/pptx/windows-powershell.ts b/src/main/pptx/windows-powershell.ts index b71dcb7..756ad89 100644 --- a/src/main/pptx/windows-powershell.ts +++ b/src/main/pptx/windows-powershell.ts @@ -8,6 +8,7 @@ import path from 'path'; 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 currentAnimationStep = 0; let totalSlides = 1; let localPresentationCopy = ''; -export const windowsAutomation: PowerPointAutomation = { +const powerShellBackend: PowerPointAutomation = { async checkInstalled() { try { log.info('[Windows] Checking if PowerPoint is installed'); @@ -378,10 +379,11 @@ export const windowsAutomation: PowerPointAutomation = { 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 @@ export const windowsAutomation: PowerPointAutomation = { 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 @@ export const windowsAutomation: PowerPointAutomation = { 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 @@ export const windowsAutomation: PowerPointAutomation = { // 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 @@ export const windowsAutomation: PowerPointAutomation = { } }, }; + +// 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); diff --git a/src/main/pptx/windows-winax.ts b/src/main/pptx/windows-winax.ts index 9df92b1..3692864 100644 --- a/src/main/pptx/windows-winax.ts +++ b/src/main/pptx/windows-winax.ts @@ -5,6 +5,7 @@ import { join, basename } from 'path'; 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 @@ -40,7 +41,7 @@ function queryCurrentSlide(): number { 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 @@ export const windowsWinaxAutomation: PowerPointAutomation = { 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 @@ export const windowsWinaxAutomation: PowerPointAutomation = { 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 @@ export const windowsWinaxAutomation: PowerPointAutomation = { } }, }; + +// 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); diff --git a/src/main/server/socket.ts b/src/main/server/socket.ts index f83c19e..20f102c 100644 --- a/src/main/server/socket.ts +++ b/src/main/server/socket.ts @@ -2,9 +2,11 @@ // SPDX-FileCopyrightText: 2025-2026 Schuberg Philis / Lab271 import { Server } from 'socket.io'; import { getAutomation } from '../pptx/automation'; +import { PowerPointAutomation } from '../pptx/types'; import log from 'electron-log'; let pollInterval: NodeJS.Timeout | null = null; +let pollInFlight: Promise | null = null; let lastSlideNumber = 1; export function setupSocketHandlers(io: Server) { @@ -60,29 +62,41 @@ export function setupSocketHandlers(io: Server) { }); }); } -function startSlidePolling(io: Server, automation: any) { +function startSlidePolling(io: Server, automation: PowerPointAutomation) { // Stop existing poll if any if (pollInterval) { clearInterval(pollInterval); } - // Poll every 500ms for slide changes - pollInterval = setInterval(async () => { - try { - const info = await automation.getSlideInfo(); - - // Only emit if slide actually changed - if (info.currentSlide !== lastSlideNumber) { - log.info(`Slide changed from ${lastSlideNumber} to ${info.currentSlide}`); - lastSlideNumber = info.currentSlide; - io.emit('slide-changed', info); - } - } catch { - // Ignore errors during polling + // Poll every 500ms for slide changes. Automation calls are serialized per + // backend, so a poll that outlives its tick would queue behind the next one + // and behind every remote command instead of overlapping with them - skip the + // tick rather than let the backlog grow. + pollInterval = setInterval(() => { + if (pollInFlight) { + return; } + pollInFlight = pollOnce(io, automation).finally(() => { + pollInFlight = null; + }); }, 500); } +async function pollOnce(io: Server, automation: PowerPointAutomation): Promise { + try { + const info = await automation.getSlideInfo(); + + // Only emit if slide actually changed + if (info.currentSlide !== lastSlideNumber) { + log.info(`Slide changed from ${lastSlideNumber} to ${info.currentSlide}`); + lastSlideNumber = info.currentSlide; + io.emit('slide-changed', info); + } + } catch { + // Ignore errors during polling + } +} + export function stopSlidePolling() { if (pollInterval) { clearInterval(pollInterval); diff --git a/test/serialize.test.ts b/test/serialize.test.ts new file mode 100644 index 0000000..88e798b --- /dev/null +++ b/test/serialize.test.ts @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2025-2026 Schuberg Philis / Lab271 +import { describe, it, expect } from 'vitest'; +import { serializeAutomation } from '../src/main/pptx/serialize'; +import type { PowerPointAutomation, SlideInfo, SlideMetadata } from '../src/main/pptx/types'; + +const tick = () => new Promise((resolve) => setTimeout(resolve, 0)); + +const emptyInfo: SlideInfo = { + currentSlide: 1, + totalSlides: 1, + animationStep: 0, + animationsOnSlide: 0, + nextVisibleSlide: null, + isLastSlide: true, + currentNotes: '', + nextNotes: '', +}; + +const emptyMetadata: SlideMetadata = { + thumbnails: [], + totalSlides: 1, + hiddenSlides: [], + visibleSlides: [], +}; + +/** A backend whose methods all resolve, for tests that only care about one of them. */ +function stubBackend(overrides: Partial = {}): PowerPointAutomation { + return { + checkInstalled: async () => true, + openPresentation: async () => {}, + exportThumbnails: async () => emptyMetadata, + startSlideshow: async () => {}, + nextSlide: async () => {}, + prevSlide: async () => {}, + gotoSlide: async () => {}, + getSlideInfo: async () => emptyInfo, + stopSlideshow: async () => {}, + closePresentation: async () => {}, + ...overrides, + }; +} + +/** + * Stands in for a PowerPoint backend: `devicePosition` is what PowerPoint + * itself is showing, `currentSlide` is the cached copy the backend keeps. The + * cached value is read before the first await and written after the second, + * which is the shape every real backend has. + */ +function fakePowerPoint() { + const seen = { maxConcurrent: 0, inFlight: 0, order: [] as string[] }; + let devicePosition = 1; + let currentSlide = 1; + + async function command(label: string, move: (from: number) => number): Promise { + seen.inFlight += 1; + seen.maxConcurrent = Math.max(seen.maxConcurrent, seen.inFlight); + seen.order.push(label); + try { + const before = currentSlide; + await tick(); // round trip out to PowerPoint + devicePosition = move(before); + await tick(); // round trip back + // Reproducing the hazard on purpose - this is the shape serializeAutomation + // exists to make safe, so the rule that bans it elsewhere is off here. + // eslint-disable-next-line require-atomic-updates + currentSlide = devicePosition; + } finally { + seen.inFlight -= 1; + } + } + + const backend = stubBackend({ + nextSlide: () => command('next', (from) => from + 1), + prevSlide: () => command('prev', (from) => Math.max(1, from - 1)), + getSlideInfo: async () => { + await tick(); + return { ...emptyInfo, currentSlide }; + }, + }); + + return { backend, seen, position: () => currentSlide }; +} + +describe('serializeAutomation', () => { + it('lets an unserialized backend lose an update (the bug being fixed)', async () => { + const { backend } = fakePowerPoint(); + + await Promise.all([backend.nextSlide(), backend.nextSlide()]); + + // Both calls read slide 1 before their await, so both advanced to 2. + expect((await backend.getSlideInfo()).currentSlide).toBe(2); + }); + + it('applies concurrent commands one at a time', async () => { + const { backend, seen } = fakePowerPoint(); + const automation = serializeAutomation(backend); + + await Promise.all([automation.nextSlide(), automation.nextSlide()]); + + expect(seen.maxConcurrent).toBe(1); + expect((await automation.getSlideInfo()).currentSlide).toBe(3); + }); + + it('runs commands in the order they were requested', async () => { + const { backend, seen } = fakePowerPoint(); + const automation = serializeAutomation(backend); + + await Promise.all([ + automation.nextSlide(), + automation.nextSlide(), + automation.prevSlide(), + automation.nextSlide(), + ]); + + expect(seen.order).toEqual(['next', 'next', 'prev', 'next']); + expect((await automation.getSlideInfo()).currentSlide).toBe(3); + }); + + it('keeps a polling read from interleaving with a command', async () => { + const { backend, seen } = fakePowerPoint(); + const automation = serializeAutomation(backend); + + const [, info] = await Promise.all([automation.nextSlide(), automation.getSlideInfo()]); + + expect(seen.maxConcurrent).toBe(1); + // The poll was queued behind the command, so it reports the settled position. + expect(info.currentSlide).toBe(2); + }); + + it('delivers a rejection only to its own caller and keeps the queue moving', async () => { + const calls: string[] = []; + const automation = serializeAutomation( + stubBackend({ + startSlideshow: async () => { + calls.push('start'); + throw new Error('PowerPoint said no'); + }, + nextSlide: async () => { + calls.push('next'); + }, + }) + ); + + const failing = automation.startSlideshow(); + const following = automation.nextSlide(); + + await expect(failing).rejects.toThrow('PowerPoint said no'); + await expect(following).resolves.toBeUndefined(); + expect(calls).toEqual(['start', 'next']); + }); + + it('forwards arguments and return values', async () => { + const opened: string[] = []; + const progress: Array<[number, number]> = []; + const metadata: SlideMetadata = { ...emptyMetadata, totalSlides: 7 }; + + const automation = serializeAutomation( + stubBackend({ + openPresentation: async (filePath) => { + opened.push(filePath); + }, + exportThumbnails: async (outputDir, onProgress) => { + onProgress?.(1, 2); + return { ...metadata, thumbnails: [`${outputDir}/slide_001.png`] }; + }, + gotoSlide: async (index) => { + opened.push(`goto:${index}`); + }, + }) + ); + + await automation.openPresentation('/tmp/deck.pptx'); + await automation.gotoSlide(4); + const result = await automation.exportThumbnails('/tmp/thumbs', (current, total) => + progress.push([current, total]) + ); + + expect(opened).toEqual(['/tmp/deck.pptx', 'goto:4']); + expect(progress).toEqual([[1, 2]]); + expect(result.totalSlides).toBe(7); + expect(result.thumbnails).toEqual(['/tmp/thumbs/slide_001.png']); + }); +});