Skip to content

[codex] Fix PowerPoint slide sync detection#21

Merged
EinfachMxrc merged 1 commit into
mainfrom
codex/fix-slide-sync-detection
Apr 10, 2026
Merged

[codex] Fix PowerPoint slide sync detection#21
EinfachMxrc merged 1 commit into
mainfrom
codex/fix-slide-sync-detection

Conversation

@EinfachMxrc

@EinfachMxrc EinfachMxrc commented Apr 10, 2026

Copy link
Copy Markdown
Owner

What changed

  • fixed slide number normalization so the web PowerPoint bridge reports the correct 1-based slide index
  • made the PowerPoint add-in bridge more robust by syncing once on startup, polling as a fallback, and reacting to active view changes
  • fixed the add-in type setup so the touched packages type-check cleanly again

Why

The automatic slide detection was not reliably recognizing the current slide. One bridge returned the wrong slide index, and the add-in relied too heavily on a single Office event that does not always fire when expected.

Impact

Presenters should now see the current slide update more reliably in the PowerPoint integration, especially when opening the add-in or switching views.

Validation

  • pnpm --filter web type-check
  • pnpm --filter powerpoint-addin type-check

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed slide number calculation to display correct numbering.
    • Improved slide synchronization reliability with automatic periodic updates.
    • Enhanced error handling and reporting when synchronization encounters issues.

@netlify

netlify Bot commented Apr 10, 2026

Copy link
Copy Markdown

Deploy Preview for handoutmarc ready!

Name Link
🔨 Latest commit d20d12f
🔍 Latest deploy log https://app.netlify.com/projects/handoutmarc/deploys/69d8b574a874270008205be8
😎 Deploy Preview https://deploy-preview-21--handoutmarc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Apr 10, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ca7d6c14-3d6d-42c6-8f8f-43e9617ecc62

📥 Commits

Reviewing files that changed from the base of the PR and between 04beffc and d20d12f.

📒 Files selected for processing (4)
  • apps/powerpoint-addin/src/App.tsx
  • apps/powerpoint-addin/src/lib/officeBridge.ts
  • apps/powerpoint-addin/tsconfig.json
  • apps/web/src/lib/powerpoint/officeBridge.ts

📝 Walkthrough

Walkthrough

Enhances PowerPoint add-in slide synchronization with explicit Convex type casting, introduces polling-based state updates with hybrid mode fallback support, adds Vite client type definitions to tsconfig, and corrects slide number calculation from zero-based to one-based indexing.

Changes

Cohort / File(s) Summary
Session typing & configuration
apps/powerpoint-addin/src/App.tsx, apps/powerpoint-addin/tsconfig.json
Added typed import for Convex Id type and cast sessionId to Id<"presentationSessions"> for stricter type checking; expanded TypeScript global type definitions to include vite/client.
Slide synchronization enhancements
apps/powerpoint-addin/src/lib/officeBridge.ts
Introduced periodic polling of slide state (POLL_MS = 1000), immediate sync on Office initialization, refactored event handlers with debouncing, added ActiveViewChanged handler registration, implemented hybrid sync mode fallback on handler failures, and enhanced cleanup logic.
Slide number indexing fix
apps/web/src/lib/powerpoint/officeBridge.ts
Adjusted slide number computation to treat Office-reported index as zero-based by adding + 1 offset for correct slide numbering.

Sequence Diagram

sequenceDiagram
    participant Timer as Polling Timer
    participant OfficeBridge as Office Bridge
    participant Office as Office App
    participant Server as Convex Server
    participant UI as UI Callbacks

    rect rgba(100, 150, 200, 0.5)
    Note over OfficeBridge,UI: Initialization
    OfficeBridge->>Office: Initialize Office Bridge
    OfficeBridge->>OfficeBridge: Start polling (1s interval)
    OfficeBridge->>OfficeBridge: syncCurrentSlide()
    OfficeBridge->>Office: getCurrentSlideInfo()
    Office-->>OfficeBridge: Slide info
    OfficeBridge->>Server: Update current slide
    OfficeBridge->>UI: onModeChange("auto")
    end

    rect rgba(150, 200, 100, 0.5)
    Note over Timer,Server: Polling Loop
    loop Every 1000ms
        Timer->>OfficeBridge: Poll tick
        OfficeBridge->>OfficeBridge: syncCurrentSlide()
        OfficeBridge->>Office: getCurrentSlideInfo()
        Office-->>OfficeBridge: Slide info
        OfficeBridge->>Server: Update if changed
    end
    end

    rect rgba(200, 150, 100, 0.5)
    Note over OfficeBridge,UI: Fallback on Error
    OfficeBridge->>Office: Register event handler
    Office--XOfficeBridge: Handler registration fails
    OfficeBridge->>UI: onModeChange("hybrid")
    Note over OfficeBridge: Continue polling, disable auto-sync
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

Hop, hop, poll the slide track,
Type-safe casting brings it back,
Hybrid sync with timers bound,
Polling keeps us spinning 'round,
Office bridges strengthened bright! 🐰

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-slide-sync-detection

Comment @coderabbitai help to get the list of available commands and usage tips.

@EinfachMxrc EinfachMxrc marked this pull request as ready for review April 10, 2026 08:32
@EinfachMxrc EinfachMxrc merged commit 0443360 into main Apr 10, 2026
4 of 5 checks passed
@greptile-apps

greptile-apps Bot commented Apr 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes PowerPoint add-in slide synchronization by correcting the 0-based-to-1-based slide index conversion (slides[0].index + 1), adding an immediate startup sync, a 1-second polling fallback, and an ActiveViewChanged event handler. It also aligns the web bridge to match the addin and fixes tsconfig.json to include office-js and vite/client types.

The changes are well-motivated and the core fix is correct. A few issues warrant attention:

  • The addin bridge's destroyOfficeBridge has an early-return path (when isOfficeInitialized is still false) that does not clear _callbacks or _lastReportedSlide, leaving stale callbacks reachable by a deferred onReady invocation.
  • The web bridge's getCurrentSlideInfo does not protect the getSlideCountAsync call inside the async getSelectedDataAsync callback with a try/catch, so a TypeError there leaves the returned Promise permanently pending.
  • In App.tsx, syncSlide is captured by the useEffect callback but not included in the dependency array.

Confidence Score: 3/5

Core index fix is correct but a stale-callback bug in the addin bridge teardown path should be resolved before merge.

The P1 in apps/powerpoint-addin/src/lib/officeBridge.ts — stale _callbacks surviving the early-return in destroyOfficeBridge — can cause Convex mutations to fire against a session already switched away from, which is the primary user path this PR is meant to fix. One targeted fix in the addin file and an error guard in the web bridge are needed before this is reliably safe to merge.

Primary: apps/powerpoint-addin/src/lib/officeBridge.ts (destroyOfficeBridge early-return). Secondary: apps/web/src/lib/powerpoint/officeBridge.ts (getSlideCountAsync exception guard).

Important Files Changed

Filename Overview
apps/powerpoint-addin/src/lib/officeBridge.ts Adds startup sync, polling fallback, and ActiveViewChanged handler — but destroyOfficeBridge skips clearing _callbacks/_lastReportedSlide in the early-return path, leaving stale callbacks reachable after the component has already unmounted.
apps/web/src/lib/powerpoint/officeBridge.ts Mirrors the addin bridge with the correct index+1 fix and polling; destroyOfficeBridge properly clears callbacks even on early return, but getSlideCountAsync is not guarded by try/catch inside the async callback.
apps/powerpoint-addin/src/App.tsx Correctly wires the updated bridge; minor React exhaustive-deps violation (syncSlide used inside useEffect but not listed in dependency array).
apps/powerpoint-addin/tsconfig.json Adds office-js and vite/client to explicit types array, fixing type-check for the addin package.

Sequence Diagram

sequenceDiagram
    participant App as App.tsx / PowerPointAddinClient
    participant Bridge as officeBridge.ts
    participant Office as Office.js
    participant Convex as Convex (setCurrentSlide)

    App->>Bridge: initOfficeBridge(callbacks)
    Bridge->>Office: Office.onReady()
    Office-->>Bridge: onReady callback fires
    Bridge->>Bridge: isOfficeInitialized = true
    Bridge->>Bridge: syncCurrentSlide() [startup sync]
    Bridge->>Bridge: startPolling() [1s interval fallback]
    Bridge->>Office: addHandlerAsync(DocumentSelectionChanged)
    Bridge->>Office: addHandlerAsync(ActiveViewChanged)
    Office-->>Bridge: handler registered
    Bridge-->>App: Promise resolves with auto/hybrid

    loop Every 1 second (polling fallback)
        Bridge->>Office: getSelectedDataAsync(SlideRange)
        Office-->>Bridge: slides[0].index (0-based)
        Bridge->>Bridge: slideNumber = slides[0].index + 1
        Bridge->>Office: getSlideCountAsync()
        Office-->>Bridge: totalSlides
        Bridge->>Bridge: compare with _lastReportedSlide
        alt slide changed
            Bridge-->>App: onSlideChange(info)
            App->>Convex: setCurrentSlide(slideNumber)
        end
    end

    Note over Bridge,Office: Also fires on DocumentSelectionChanged and ActiveViewChanged (debounced 250ms)

    App->>Bridge: destroyOfficeBridge() [on unmount]
    Bridge->>Bridge: clearInterval / clearTimeout
    Bridge->>Office: removeHandlerAsync(DocumentSelectionChanged)
    Bridge->>Office: removeHandlerAsync(ActiveViewChanged)
Loading

Comments Outside Diff (3)

  1. apps/powerpoint-addin/src/lib/officeBridge.ts, line 211-235 (link)

    P1 Stale callbacks survive early return in destroyOfficeBridge

    When destroyOfficeBridge is called before Office.onReady() has fired, isOfficeInitialized is still false and the function returns early on line 220 — without clearing _callbacks or _lastReportedSlide. The Office.onReady() callback registered in initOfficeBridge is still pending in Office.js's queue and will fire later, at which point syncCurrentSlide() runs with the stale _callbacks pointing to the already-torn-down component. This can cause onSlideChange to dispatch a Convex setCurrentSlide mutation against a session that's no longer active.

    The web version (apps/web/src/lib/powerpoint/officeBridge.ts) handles this correctly by always nulling out activeCallbacks even in the early-return path. The fix is to move the cleanup before the guard:

    Fix in Codex

  2. apps/web/src/lib/powerpoint/officeBridge.ts, line 160-168 (link)

    P2 getSlideCountAsync exception inside async callback leaves Promise pending

    getSlideCountAsync is invoked via an as any cast inside the getSelectedDataAsync callback. If the API is not present in a particular Office version and throws a TypeError, the exception occurs inside an async callback — outside the surrounding try/catch that only wraps the synchronous getSelectedDataAsync call. The Promise returned by getCurrentSlideInfo will never settle, and since syncCurrentSlide is fired from a setInterval via void syncCurrentSlide(), a new unresolved Promise accumulates every second.

    The addin version avoids this by wrapping the inner callback body in its own try/catch. Apply the same guard here:

    Fix in Codex

  3. apps/powerpoint-addin/src/App.tsx, line 38-68 (link)

    P2 syncSlide used inside useEffect but omitted from dependency array

    syncSlide is defined in the component body (line 70) and called inside the onSlideChange callback, but it is not listed in the useEffect dependency array. This violates the React exhaustive-deps rule. In practice it is safe because every variable syncSlide closes over (convexReady, store.sessionId, store.presenterToken, setCurrentSlide) is already in the dep array or is a stable Convex mutation reference. Consider wrapping syncSlide in useCallback with the relevant deps, or restructuring the callbacks to reference state via refs, to keep the linter happy and make the intent explicit.

    Fix in Codex

Fix All in Codex

Reviews (1): Last reviewed commit: "Fix PowerPoint slide sync detection" | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant