fix(vscode): retry webview load on service worker failure#8514
fix(vscode): retry webview load on service worker failure#8514markijbema wants to merge 3 commits into
Conversation
Mitigate VS Code platform bug (microsoft/vscode#125993) where the internal service worker fails to register, leaving the webview blank. After setting webview.html, start an 8s timer. If webviewReady never arrives, re-assign the HTML to force a fresh iframe + service worker registration. Retry up to 3 times, then show a 'Reload Window' prompt. Covers sidebar, tab panels, Agent Manager, and DiffViewer. Closes #8512
Code ReviewReviewer: AI (assisted by kilo-auto/free model) SummaryThis PR adds retry logic to handle VS Code's service worker registration failures (referenced issue: microsoft/vscode#125993). The fix is applied to three files: , , and . Strengths
Issues to Address
VerdictApprove with minor suggestions. The implementation is solid and handles the known VS Code issue well. The main concern is code duplication - a shared utility would be more maintainable. The wiring in should be verified to ensure the correct provider's is being called. |
There was a problem hiding this comment.
Code Review — requested by @markijbema
Reviewed by: Kilo Code Agent (model: anthropic/claude-opus-4.6)
Summary
The core approach is sound — re-assigning webview.html to force a fresh iframe + service worker registration is the correct workaround per VS Code's own guidance. The linked issue (#8512) has excellent root cause analysis, and the upstream references are thorough. That said, there are several issues worth addressing before merge.
Issues
1. Code duplication — retry logic copy-pasted across two classes (High)
DiffViewerProvider and KiloProvider both implement identical retry machinery (scheduleReadyCheck, retryWebview, cancelReadyCheck, plus 5 instance/static properties). That's ~40 lines duplicated. This is clearly reusable (used in 3 surfaces) and should be extracted into a shared utility or mixin.
2. DiffViewerProvider.retryWebview() silently gives up — no user notification (Medium)
In KiloProvider, when retries are exhausted, the user sees a "Reload Window" warning notification. In DiffViewerProvider, it just logs to the output channel and stops. Users of the DiffViewer will see a blank panel with no indication of what happened or what to do. This should show the same notification.
3. scheduleReadyCheck is public on KiloProvider (Medium)
It's made public so VscodeHost can call it, but it exposes internal retry state management as public API. The html parameter accepting a closure that captures panel.webview also creates a subtle lifetime concern — if the panel is disposed between retry attempts, the closure still references the old webview. retryWebview checks !this.webview, but that's KiloProvider's own stored reference, not necessarily the one captured in the closure.
4. Race condition: retry timer fires during normal slow load (Medium)
8 seconds is a fixed timeout. On slow machines or under memory pressure (exactly the conditions that trigger this bug), a legitimate first load could exceed 8 seconds. The retry would blow away a webview that was about to become ready. Consider exponential backoff, or at least increasing the timeout on subsequent retries.
5. No tests (Medium)
This is reliability-critical retry logic with timers and multiple state transitions across 3 code paths. At minimum, unit tests should verify:
- Timer is cancelled when
"webviewReady"arrives - Retries increment and cap at
MAX_RETRIES dispose()cancels the timer- The notification appears after exhausting retries
6. ready flag coupling is fragile (Low)
DiffViewerProvider.wirePanel() sets this.ready = false before calling scheduleReadyCheck(), but scheduleReadyCheck() itself doesn't reset the ready flag. If scheduleReadyCheck were ever called without the caller resetting the flag first, retryWebview would immediately no-op. Same pattern exists in KiloProvider where isWebviewReady is reset by the caller, not by scheduleReadyCheck. If this gets extracted into a shared utility (per #1), the utility should own the reset.
7. Naming inconsistency: ready vs isWebviewReady (Low)
DiffViewerProvider uses this.ready, KiloProvider uses this.isWebviewReady. Makes the duplicated logic harder to follow and extract. Per the project style guide's single-word preference, ready is better — but consistency matters more.
What's good
- Problem is well-documented with upstream issue references in every comment and log line
- The workaround approach (re-assigning
webview.html) is correct - Timer cleanup is properly handled in all dispose paths
- The
KiloProviderexhaustion path gives users a clear action ("Reload Window") - Clean integration into the existing
wirePanel/resolveWebviewView/resolveWebviewPanelflows
Recommendation
Extract the retry logic into a shared utility, add the user notification to DiffViewerProvider, and add basic test coverage. The core approach is solid — this is mostly about reducing duplication and closing the gaps.
Code Review (kilo/kilo-auto/balanced model)Verdict: Request changes before mergeThe fix correctly addresses VS Code service worker bug #125993, but there are maintainability concerns: Critical Issues1. Code duplication — The retry logic is copy-pasted nearly verbatim in 2. No test coverage — 118 lines of new retry logic with zero tests. This is high-risk code touching webview lifecycle and will be a regression surface. 3. Race condition — In Minor Issues
What's Good
|
- add WebviewReadyRetry utility - integrate into DiffViewerProvider, KiloProvider, and vscode-host - replace legacy ready timer logic with the WebviewReadyRetry loader - emit WEBVIEW_READY_RETRY and WEBVIEW_READY_FAILED telemetry events - update telemetry types and add unit tests for webview-ready-retry
Summary
webview.html, an 8-second timer monitors forwebviewReady. If it never arrives, re-assigns the HTML to force a fresh iframe + service worker registration attempt. Retries up to 3 times, then shows a "Reload Window" notification.Closes #8512