fix: multi-strategy PowerPoint slide detection#26
Conversation
Previous debounce of 150ms caused getSelectedDataAsync to be called after focus returned to the taskpane, making it always fail. Changes: - Remove debounce: call syncCurrentSlide immediately on selection change so the presentation still has focus when getSelectedDataAsync is called - Add window blur/focus listeners: blur fires when user clicks into presentation (good time to read slide), focus fires when returning - Poll every 500ms (was 800ms) as reliable fallback - PowerPoint.run + getSelectedSlides as primary (focus-independent) - Pass knownTotalSlides from PowerPoint.run into legacy fallback to avoid redundant getSlideCountAsync call - Add teardownWindowListeners in destroyOfficeBridge Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Office Bridge module is refactored to remove debouncing from selection-change handling, reduce polling interval from 800ms to 500ms, introduce window focus/blur listeners for slide synchronization, improve error-handling behavior, and enhance cleanup paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR refactors Key changes:
Confidence Score: 4/5Safe to merge — the multi-strategy approach correctly addresses both root causes, and all identified concerns are non-blocking style issues. The two diagnosed root causes (premature Office.context check, debounce-induced focus loss) are properly fixed. The three fallback strategies are logically sound and PowerPointAddinClient.tsx was already updated correctly. Remaining comments are P2: untracked blur timeouts, missing double-registration guard on window listeners, and silent error suppression — none of these break the primary user path or introduce data loss. apps/web/src/lib/powerpoint/officeBridge.ts — specifically setupWindowListeners (double-registration) and the blur timeout lifecycle. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as PowerPointAddinClient
participant Bridge as officeBridge.ts
participant OfficeJS as Office.js Runtime
participant PPAPI as PowerPoint JS API
App->>Bridge: initOfficeBridge(callbacks)
Note over Bridge: typeof Office === "undefined"?<br/>→ resolve("manual_only")
Bridge->>OfficeJS: Office.onReady()
OfficeJS-->>Bridge: {host: PowerPoint}
Bridge->>Bridge: isInitialized = true
Bridge->>Bridge: syncCurrentSlide() [immediate]
Bridge->>Bridge: startPolling() [every 500ms]
Bridge->>Bridge: setupWindowListeners()
Note over Bridge: window.addEventListener(blur)<br/>window.addEventListener(focus)
Bridge->>OfficeJS: addHandlerAsync(DocumentSelectionChanged)
OfficeJS-->>Bridge: success → resolve(auto)
Note over App,PPAPI: User clicks slide in PowerPoint
OfficeJS->>Bridge: DocumentSelectionChanged (immediate, no debounce)
Bridge->>Bridge: syncCurrentSlide()
alt PowerPoint JS API 1.5 available
Bridge->>PPAPI: PowerPoint.run(context => ...)
PPAPI->>Bridge: context.presentation.getSelectedSlides()
Bridge-->>App: onSlideChange(slideNumber, totalSlides)
else Fallback: legacyGetSlideInfo
Bridge->>OfficeJS: getSelectedDataAsync(SlideRange)
OfficeJS-->>Bridge: slides[0].index
Bridge->>OfficeJS: getSlideCountAsync()
OfficeJS-->>Bridge: totalSlides
Bridge-->>App: onSlideChange(slideNumber, totalSlides)
end
Note over Bridge: window blur (taskpane loses focus)
Bridge->>Bridge: setTimeout(syncCurrentSlide, 80ms)
App->>Bridge: destroyOfficeBridge()
Bridge->>Bridge: clearInterval(pollTimer)
Bridge->>Bridge: teardownWindowListeners()
Bridge->>OfficeJS: removeHandlerAsync(DocumentSelectionChanged)
Bridge->>OfficeJS: removeHandlerAsync(ActiveViewChanged)
Bridge->>Bridge: isInitialized = false
Reviews (1): Last reviewed commit: "fix: multi-strategy slide detection with..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function setupWindowListeners() { | ||
| if (typeof window === "undefined") return; | ||
|
|
||
| // Taskpane lost focus → user moved to presentation. |
There was a problem hiding this comment.
Each blur event creates a new setTimeout whose ID is never stored, so destroyOfficeBridge (and teardownWindowListeners) cannot cancel pending timers. If the bridge is torn down while an 80ms timer is still queued, syncCurrentSlide will run after activeCallbacks is set to null. The function already guards against a null activeCallbacks, so there's no crash — but unnecessary async work is triggered after teardown, and if many blur events fire in quick succession (user rapidly alt-tabs) multiple timers queue up.
Consider storing and cancelling the ID:
| // Taskpane lost focus → user moved to presentation. | |
| let blurTimer: ReturnType<typeof setTimeout> | null = null; | |
| windowBlurListener = () => { | |
| if (blurTimer) clearTimeout(blurTimer); | |
| blurTimer = setTimeout(() => { blurTimer = null; void syncCurrentSlide(); }, 80); | |
| }; |
Alternatively, promote blurTimer to module scope alongside windowBlurListener and clear it in teardownWindowListeners.
| } | ||
|
|
||
| function setupWindowListeners() { | ||
| if (typeof window === "undefined") return; | ||
|
|
||
| // Taskpane lost focus → user moved to presentation. | ||
| // Wait briefly for focus transfer to complete, then try reading. | ||
| windowBlurListener = () => setTimeout(() => void syncCurrentSlide(), 80); | ||
|
|
||
| // Taskpane gained focus → user just came from the slides. | ||
| // Read immediately before focus fully transfers to taskpane. | ||
| windowFocusListener = () => void syncCurrentSlide(); | ||
|
|
||
| window.addEventListener("blur", windowBlurListener, { passive: true }); | ||
| window.addEventListener("focus", windowFocusListener, { passive: true }); | ||
| } | ||
|
|
||
| function teardownWindowListeners() { | ||
| if (typeof window === "undefined") return; | ||
| if (windowBlurListener) { |
There was a problem hiding this comment.
setupWindowListeners has no guard against double-registration
setupWindowListeners unconditionally overwrites windowBlurListener / windowFocusListener and calls addEventListener without first checking whether listeners are already attached. If initOfficeBridge is called a second time (e.g., React StrictMode double-invoke in development, or session re-initialization without a prior destroyOfficeBridge), both the old and new listener functions are registered on the window — teardownWindowListeners will only remove the second one because the module-level variable now points to it.
A simple guard prevents this:
| } | |
| function setupWindowListeners() { | |
| if (typeof window === "undefined") return; | |
| // Taskpane lost focus → user moved to presentation. | |
| // Wait briefly for focus transfer to complete, then try reading. | |
| windowBlurListener = () => setTimeout(() => void syncCurrentSlide(), 80); | |
| // Taskpane gained focus → user just came from the slides. | |
| // Read immediately before focus fully transfers to taskpane. | |
| windowFocusListener = () => void syncCurrentSlide(); | |
| window.addEventListener("blur", windowBlurListener, { passive: true }); | |
| window.addEventListener("focus", windowFocusListener, { passive: true }); | |
| } | |
| function teardownWindowListeners() { | |
| if (typeof window === "undefined") return; | |
| if (windowBlurListener) { | |
| function setupWindowListeners() { | |
| if (typeof window === "undefined") return; | |
| if (windowBlurListener || windowFocusListener) return; // already registered | |
| // Taskpane lost focus → user moved to presentation. | |
| // Wait briefly for focus transfer to complete, then try reading. | |
| windowBlurListener = () => setTimeout(() => void syncCurrentSlide(), 80); | |
| // Taskpane gained focus → user just came from the slides. | |
| // Read immediately before focus fully transfers to taskpane. | |
| windowFocusListener = () => void syncCurrentSlide(); | |
| window.addEventListener("blur", windowBlurListener, { passive: true }); | |
| window.addEventListener("focus", windowFocusListener, { passive: true }); | |
| } |
| if (info.slideNumber !== lastReportedSlide) { | ||
| lastReportedSlide = info.slideNumber; | ||
| activeCallbacks.onSlideChange(info); | ||
| } | ||
| } catch (error) { | ||
| activeCallbacks?.onError(`Folien-Update fehlgeschlagen: ${String(error)}`); | ||
| } catch { | ||
| // silently ignore transient errors | ||
| } | ||
| } |
There was a problem hiding this comment.
onError signal path
Previously, syncCurrentSlide forwarded exceptions to activeCallbacks?.onError(...) so the UI could surface "Folien-Update fehlgeschlagen: …". The new catch block silently discards all errors.
Since syncCurrentSlide is now called on every blur/focus/selection event AND every 500ms, making every transient failure user-visible would indeed be noisy. However, errors from the initial void syncCurrentSlide() call in initOfficeBridge (right after isInitialized = true) are also silently discarded — if something is fundamentally broken, the user gets no feedback at all.
Consider at least logging to the console in development, or throttling the error report so only the first failure per session surfaces via onError.
1 participant
Problem
Das Add-in erkannte weder die aktuelle Folie noch die Gesamtanzahl.
Ursache 1 – Office.js Timing:
isOfficeAvailable()prüfteOffice.contextbevor Office.js fertig war → Bridge wurde nie gestartet.Ursache 2 – Debounce: Die 150ms Verzögerung in
handleSelectionChangedließ den Fokus zurück zum Taskpane wandern, bevorgetSelectedDataAsyncaufgerufen wurde → API schlug immer fehl.Fixes
Bridge-Initialisierung
typeof Office !== "undefined"prüfen (Script geladen?)officeDetectedwird erst nachOffice.onReady()gesetztSlide-Erkennung (3 Strategien kombiniert)
PowerPoint.run()+getSelectedSlides()(PowerPointApi 1.5) — fokusunabhängig, primäre MethodeDocumentSelectionChangedohne Debounce — liest sofort wenn Folie geklickt, Fokus ist noch auf der Präsentationwindow blur/focusListener — zusätzliche Trigger wenn Fokus zwischen Taskpane und Präsentation wechseltNach dem Merge
Taskpane in PowerPoint schließen und neu öffnen — kein Manifest-Re-Download nötig.
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Bug Fixes