fix(pptx): serialize automation commands, and enforce require-atomic-updates - #70
Merged
Conversation
…updates Every PowerPoint backend caches the slideshow position in module-level state 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 stopped two of those from overlapping: the web remote can fire `next` from several devices at once, a double-tap queues two `nextSlide()` calls, and the 500 ms poll in server/socket.ts calls `getSlideInfo()` on top of both. `require-atomic-updates` flagged seven sites where that state is read before an await and written after it, and #64 had to land the rule as 'warn' to keep CI green. This fixes the sites and turns it up to 'error'. serializeAutomation() chains every call onto one promise, so a backend has at most one command in flight - which is also what the PowerShell bridge's FIFO response matching already assumed. Each backend exports its object already wrapped, so main/index.ts and server/socket.ts share one queue rather than getting a mutex each. Methods are listed explicitly instead of proxied, so adding an unserialized method to the interface is a type error. The mutex alone does not satisfy the lint rule, which is purely syntactic, so each flagged site also re-reads its state after the await. That is deliberate belt and braces: it keeps the invariant checkable instead of resting on the wrapper still being there. nextSlide now commits slide and animation step in one synchronous block (same result as before - the old code incremented the step then zeroed it if the slide had moved), and localPresentationCopy is cleared before the unlink is awaited rather than after. The 500 ms poll needed a guard as a direct consequence: overlapping polls used to just run concurrently, but under serialization a poll that outlives its tick queues behind the next poll and every remote command, so a slow backend would grow an unbounded backlog. It now skips a tick while one is outstanding. Verified against a real PowerPoint 16.112.3 on macOS with a generated six-slide deck (slide 3 hidden): openPresentation, gotoSlide, getSlideInfo, closePresentation, concurrent and burst navigation, and the temp-copy cleanup. With the serializer stubbed out, two concurrent gotoSlide calls interleave and one is silently dropped - PowerPoint stays on slide 5 after a request for slide 2. With it in place the commands apply in order. Not verified against a real install: startSlideshow, nextSlide, prevSlide and stopSlideshow on macOS go through System Events keystrokes, and Accessibility permission was not granted to the test process. Neither Windows backend was run at all; their changes are the same three patterns applied identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the
require-atomic-updatesdebt that #64 had to park as'warn'to get linting turned on at all.The problem
Every PowerPoint backend caches the slideshow position in module-level state (
currentSlide,currentAnimationStep,localPresentationCopy) and refreshes it from PowerPoint across anawait— an AppleScript round trip on macOS, a PowerShell bridge command or a COM call on Windows. Nothing stopped two of those from overlapping:nextfrom several devices at once,nextSlide()calls,server/socket.tscallsgetSlideInfo()on top of both.The lint rule pointed at the narrow symptom — a write based on a stale pre-
awaitread. The live testing below turned up a worse one: a navigation command gets silently dropped.The fix
Serialize the commands. New
pptx/serialize.ts:serializeAutomation()chains every call onto a single promise, so a backend has at most one command in flight. That is also what the PowerShell bridge's FIFO response matching already assumed. Each backend exports its object already wrapped, somain/index.tsandserver/socket.tsshare one queue instead of getting a mutex each — they callgetAutomation()separately. Methods are listed explicitly rather than proxied, so adding an unserialized method to the interface is a type error. A rejected call doesn't stall the queue and its error reaches only its own caller.Re-read state after the await at each of the seven sites. The mutex alone does not satisfy the rule — it's purely syntactic — but this is not just linter appeasement: it keeps the invariant checkable rather than resting on the wrapper still being there.
nextSlidenow commits slide and animation step in one synchronous block (same result as before: the old code incremented the step, then zeroed it if the slide had moved), andlocalPresentationCopyis cleared before the unlink is awaited, so the field never points at a file already on its way out.Guard the poll. A direct consequence of serializing: overlapping polls used to just run concurrently, but now a poll that outlives its tick queues behind the next poll and every remote command, so a slow backend would grow an unbounded backlog. It now skips a tick while one is outstanding.
startSlidePollingalso lost itsany.Then
'require-atomic-updates': 'error', and the waiver comment is gone.Verification
Lint (0 errors, was 7), typecheck, build, and 13 unit tests pass.
test/serialize.test.tscovers ordering, one-at-a-time execution, poll/command isolation, rejection isolation and argument pass-through, plus an unserialized control that loses an update. Stubbingrunto a passthrough fails exactly the 3 concurrency tests, so they aren't vacuous.Live against real PowerPoint 16.112.3 on macOS, with a generated six-slide deck (slide 3 hidden, notes on every slide). Covered for real:
openPresentation,gotoSlide,getSlideInfo,closePresentation, concurrent and burst navigation, hidden-slide skipping, and the temp-copy cleanup. 9/9 pass.The counterfactual is the interesting part. With the serializer stubbed out, two concurrent
gotoSlidecalls interleave and one is silently dropped:PowerPoint stayed on slide 5 after a request for slide 2. With the serializer in place the calls pair up cleanly and the deck lands on 2.
What is not verified
Worth weighing before merge — a green CI check here proves the unit tests, lint and typecheck, nothing about a real install:
startSlideshow,nextSlide,prevSlide,stopSlideshowgo through System Events, and Accessibility permission was not granted to the test process (osascripterror 1002). The slideshow in the live run was started via a pure Apple Events substitute in the harness, not viastartSlideshow(). So thecurrentAnimationStepfix innextSlidehas unit coverage but no live coverage.🤖 Generated with Claude Code