Skip to content

fix: correct slide detection for PowerPoint Online#27

Merged
EinfachMxrc merged 1 commit into
mainfrom
fix/office-init-timing
Apr 11, 2026
Merged

fix: correct slide detection for PowerPoint Online#27
EinfachMxrc merged 1 commit into
mainfrom
fix/office-init-timing

Conversation

@EinfachMxrc

@EinfachMxrc EinfachMxrc commented Apr 11, 2026

Copy link
Copy Markdown
Owner

Problem

In PowerPoint Online (SharePoint/OneDrive) blieb die angezeigte Folie dauerhaft auf 1.

Ursache: PowerPoint.run() + getSelectedSlides() gibt in PowerPoint Online immer Folie 1 zurück, unabhängig davon welche Folie gerade angezeigt wird. Diese Methode war bisher als primäre Strategie eingestellt.

Fix

  • getSelectedDataAsync(CoercionType.SlideRange) ist jetzt die primäre Methode für alle Plattformen — funktioniert in Online zuverlässig und in Desktop wenn Fokus auf der Präsentation liegt
  • PowerPoint.run() + getSelectedSlides() nur noch als Desktop-Fallback (wird in Online via Office.PlatformType.OfficeOnline Check übersprungen)
  • Kein Debounce beim DocumentSelectionChanged Handler — liest sofort, solange die Folie noch aktiv ist

Nach dem Merge

Taskpane in PowerPoint Online schließen und neu öffnen — kein Manifest-Re-Download nötig.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

Bug Fixes

  • Improved detection of PowerPoint Online vs Desktop environments for more consistent behavior
  • Enhanced slide information retrieval with more resilient fallback handling across different PowerPoint configurations

PowerPoint.run + getSelectedSlides() returns wrong data in Online
(always slide 1). getSelectedDataAsync(SlideRange) is the correct
primary API for both Online and Desktop.

- getSelectedDataAsync is now PRIMARY for all platforms
- PowerPoint.run + getSelectedSlides kept as Desktop-only fallback
  (skipped in Online via Office.PlatformType.OfficeOnline check)
- getSlideCount extracted as helper to avoid code duplication

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vercel

vercel Bot commented Apr 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
handout-powerpoint-addin Ready Ready Preview, Comment Apr 11, 2026 0:16am

@coderabbitai

coderabbitai Bot commented Apr 11, 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: 5c5d88a0-0118-4dd9-be0c-3a514259c0b0

📥 Commits

Reviewing files that changed from the base of the PR and between e6d855b and bff7ad0.

📒 Files selected for processing (1)
  • apps/web/src/lib/powerpoint/officeBridge.ts

📝 Walkthrough

Walkthrough

Modified the PowerPoint Office Bridge's slide detection and initialization flow. Added online/offline detection via isOnline(), replaced the primary slide-info retrieval strategy to use Office.context.document.getSelectedDataAsync, introduced getSlideCount() and powerPointRunGetSlide() helper functions, and removed the legacy legacyGetSlideInfo() function with conditional fallback handling.

Changes

Cohort / File(s) Summary
Office Bridge Slide Detection Refactor
apps/web/src/lib/powerpoint/officeBridge.ts
Added isOnline() function for platform detection. Restructured getCurrentSlideInfo() to prioritize Office.context.document.getSelectedDataAsync over PowerPoint.run. Introduced getSlideCount() helper using getSlideCountAsync with fallback to slideNumber. Added powerPointRunGetSlide() as Desktop-only fallback. Removed legacyGetSlideInfo() function. Updated initialization comments and clarified window blur/focus handler rationale.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 A bridge rebuilt with Online care,
Detecting desktops floating in the air,
When slides need finding, API knows the way—
Fallbacks and helpers save the day!

✨ 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 fix/office-init-timing

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

@EinfachMxrc EinfachMxrc merged commit b7145cf into main Apr 11, 2026
2 of 3 checks passed
@EinfachMxrc EinfachMxrc deleted the fix/office-init-timing branch April 11, 2026 00:16
@greptile-apps

greptile-apps Bot commented Apr 11, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes slide detection for PowerPoint Online by promoting getSelectedDataAsync(CoercionType.SlideRange) to the primary strategy and demoting PowerPoint.run() + getSelectedSlides() to a Desktop-only fallback. The motivation is correct — getSelectedSlides() is unreliable in Online and has been confirmed to return slide 1 regardless of actual selection.

Key changes:

  • getSelectedDataAsync(SlideRange) is now the primary slide-detection method across all platforms.
  • PowerPoint.run() + getSelectedSlides() is now skipped in Online via an isOnline() guard and used only as a Desktop fallback.
  • The DocumentSelectionChanged handler fires immediately (no debounce) so the slide is read while focus is still active.
  • A 500 ms polling loop and window blur/focus listeners supplement the event-based approach.

Critical issue found:

  • The comment on line 186 claims slides[0].index is 0-based, but Microsoft's own documentation explicitly states it is 1-based. The + 1 on that line causes every reported slide number to be off by one — undermining the primary strategy this PR introduces.

Minor issues:

  • Office.context.document.url can return an empty string in some Online environments; ?? \"Praesentation\" won't catch it.
  • initOfficeBridge has no guard against being called twice, risking duplicate event handlers and redundant poll timers.

Confidence Score: 2/5

Not safe to merge as-is — the primary detection path introduced in this PR reports slide numbers that are consistently off by one on every platform.

The fix's intent is correct and the architectural restructuring (promoting getSelectedDataAsync to primary, guarding the Desktop fallback) is sound. However, the core implementation contains a critical off-by-one error: the index property from SlideRange is 1-based per Microsoft's own documentation, but the code treats it as 0-based and adds 1. This means the primary strategy — the central fix for PowerPoint Online — will always report the wrong slide number (e.g., slide 2 shows as slide 3), which breaks the user-facing feature this PR is meant to fix. The single-character fix is trivial, but the bug must be corrected before merging.

apps/web/src/lib/powerpoint/officeBridge.ts — specifically line 186-187 (off-by-one on slide index)

Important Files Changed

Filename Overview
apps/web/src/lib/powerpoint/officeBridge.ts Reworks slide detection strategy for PowerPoint Online — correct intent, but the primary path has a critical off-by-one error: slides[0].index from getSelectedDataAsync(SlideRange) is 1-based per MS docs, so adding +1 always reports one slide too high.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[syncCurrentSlide called
via poll / event / blur] --> B[getCurrentSlideInfo]
    B --> C{isInitialized &&
isOfficeAvailable?}
    C -- No --> D[resolve null]
    C -- Yes --> E[getSelectedDataAsync
SlideRange PRIMARY]
    E --> F{status !== Failed
&& slides.length > 0?}
    F -- Yes --> G["slideNumber = slides[0].index + 1
⚠️ BUG: index is 1-based,
should NOT add 1"]
    G --> H[getSlideCountAsync]
    H --> I[resolve OfficeSlideInfo]
    F -- No --> J{isOnline?}
    J -- Yes / API missing --> D
    J -- No --> K[powerPointRunGetSlide
Desktop fallback]
    K --> L[PowerPoint.run +
getSelectedSlides]
    L --> M[id-based indexOf lookup
then idx + 1]
    M --> I
Loading

Comments Outside Diff (2)

  1. apps/web/src/lib/powerpoint/officeBridge.ts, line 170 (link)

    P2 Fallback title may silently drop file extension check

    Office.context.document.url can return an empty string ("") on some Office Online environments (not just null/undefined). The nullish coalescing ?? "Praesentation" only catches null/undefined, so an empty string "" would flow through split, pop, and replace and ultimately produce an empty presentationTitle. Consider guarding against falsy values:

    Fix in Codex

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

    P2 startPolling can create a timer leak if called multiple times

    startPolling clears any existing pollTimer before starting a new one, which prevents double-polling. However, initOfficeBridge can be called multiple times (it doesn't short-circuit when isInitialized is already true). A second call would re-start polling via startPolling() and also re-register the DocumentSelectionChanged and ActiveViewChanged handlers. Consider guarding at the top of initOfficeBridge:

    Or ensure destroyOfficeBridge is always called before re-initializing.

    Fix in Codex

Fix All in Codex

Reviews (1): Last reviewed commit: "fix: prioritize getSelectedDataAsync for..." | Re-trigger Greptile

Comment on lines +186 to +187
const slideNumber = slides[0].index + 1; // index is 0-based
getSlideCount(presentationTitle, slideNumber, resolve);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 slides[0].index is 1-based, not 0-based — slide number is off by 1

The code comment says "index is 0-based" and adds + 1, but the Microsoft Office.js documentation explicitly states:

"The index value of the slide is the 1-based value the user sees beside the slide in the thumbnails pane."

So for slide 3, slides[0].index returns 3, and 3 + 1 = 4 — one slide ahead of the actual position. This means the primary detection strategy (which is supposed to fix the PowerPoint Online bug) will always report the wrong slide number on every platform.

Suggested change
const slideNumber = slides[0].index + 1; // index is 0-based
getSlideCount(presentationTitle, slideNumber, resolve);
const slideNumber = slides[0].index; // index is 1-based

Fix in Codex

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