Repository navigation
Refactor webview contexts for explicit host ownership - #9047
Open
Henning Dieterichs (hediet) wants to merge 4 commits into
Open
Henning Dieterichs (hediet) wants to merge 4 commits into
Henning Dieterichs (hediet) wants to merge 4 commits into
Conversation
Inject hosts into pull request contexts, remove context singletons, and add instance-local timestamp and viewport inputs with regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Henning Dieterichs (hediet)
October 9, 2026 16:40
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Entry points retain host subscriptions after unmount, and context-switching effects remain bound to stale instances.
4 open findings
What changed in this PR
Refactors webviews to use explicitly owned hosts and contexts, enabling isolated rendering and deterministic fixtures.
Changes:
- Introduces host-based messaging, persistence, subscriptions, and disposal.
- Adds injectable timestamp formatting and viewport-width behavior.
- Updates production entry points and tests for explicit context ownership.
| File | Description |
|---|---|
src/test/webviews/testHost.ts |
Adds managed test hosts. |
webviews/activityBarView/app.tsx |
Injects an explicit PR context. |
webviews/common/cache.ts |
Removes global state helpers. |
webviews/common/context.tsx |
Makes PR context host-backed. |
webviews/common/createContextNew.ts |
Makes create context host-backed. |
webviews/common/hooks.ts |
Adds injectable viewport width. |
webviews/common/host.ts |
Defines host and transport APIs. |
webviews/common/message.ts |
Refactors request and reply handling. |
webviews/common/test/hooks.test.tsx |
Tests viewport isolation and cleanup. |
webviews/common/test/host.test.ts |
Tests host isolation and persistence. |
webviews/components/sidebar.tsx |
Uses the shared viewport hook. |
webviews/components/test/timestamp.test.tsx |
Tests timestamp formatter isolation. |
webviews/components/timestamp.tsx |
Adds injectable timestamp formatting. |
webviews/createPullRequestViewNew/app.tsx |
Provides an explicit create context. |
webviews/createPullRequestViewNew/test/app.test.tsx |
Migrates create-view tests to test hosts. |
webviews/editorWebview/app.tsx |
Provides an explicit PR context. |
webviews/editorWebview/overview.tsx |
Uses the shared viewport hook. |
webviews/editorWebview/test/app.test.tsx |
Migrates root tests to test hosts. |
webviews/editorWebview/test/overview.test.tsx |
Migrates overview tests to test hosts. |
webviews/editorWebview/test/reviewSummary.test.tsx |
Migrates review tests to test hosts. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+15
to
+16
| const host = createWebviewHost(); | ||
| const context = new PRContext(host); |
| assert.strictEqual(listen.callCount, mount); | ||
| assert.strictEqual(host.messages.filter(message => message.command === 'ready').length, mount); | ||
| unmountComponentAtNode(app); | ||
| assert.strictEqual(host.listeners.size, mount); |
Comment on lines
+93
to
+94
| const host = createWebviewHost(); | ||
| const context = new CreatePRContextNew(host); |
Comment on lines
+18
to
+19
| const host = createWebviewHost(); | ||
| const context = new PRContext(host); |
Ross Wollman (rwoll)
previously approved these changes
Oct 9, 2026
Preserve upstream issue previews, mergeability polling, and stack refresh behavior while adapting new tests to explicit hosts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Henning Dieterichs (hediet)
October 9, 2026 16:59
View session
This was referenced Oct 9, 2026
Keep the reply-map leak fix independently reviewable and adapt its regression tests to the injected host. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Henning Dieterichs (hediet)
changed the base branch from
main
to
hediet/message-handler-reply-cleanup
October 9, 2026 17:04
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Notification requests and production mounts retain handlers indefinitely, while context subscriptions do not follow provider replacement.
8 open findings
🧠 Review effort: Balanced
| return () => { | ||
| ctx.onchange = null; | ||
| }; | ||
| }, []); |
| } | ||
| const req = String(++lastSentReq); | ||
| return new Promise<unknown>((resolve, reject) => { | ||
| this._pendingReplies.set(req, { resolve, reject }); |
| return () => { | ||
| ctx.onchange = null; | ||
| }; | ||
| }, []); |
| const host = createWebviewHost(); | ||
| const context = new PRContext(host); | ||
| render( | ||
| <PullRequestContext.Provider value={context}> |
Copilot started reviewing on behalf of
Henning Dieterichs (hediet)
October 9, 2026 17:05
View session
Connor Peet (connor4312)
approved these changes
Oct 9, 2026
Connor Peet (connor4312)
left a comment
Connor Peet (connor4312)
left a comment
Member
There was a problem hiding this comment.
Reviewed the complete diff and relevant host, context, entry-point, persistence, and lifecycle code. No actionable P1/P2 security or correctness issues found. No tests or builds were run.
Written by Connor's agent running GPT-6.1 Sol
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Notification commands accumulate unresolved entries in the pending-reply map for the webview’s lifetime.
8 open findings
🧠 Review effort: Balanced
Restore production viewport code here so the hook, consumers, and regression tests are reviewed with the fixtures that require them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Henning Dieterichs (hediet)
October 9, 2026 18:16
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Provider replacement retains stale subscriptions, and production-owned hosts remain subscribed after unmount.
8 open findings
🧠 Review effort: Balanced
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.
Stack
Depends on #9050 (standalone pending-reply Map/leak fix). This PR targets
hediet/message-handler-reply-cleanupso the reply-map conversion and completed-request cleanup are not part of this review diff. Merge #9050 first, then retarget this PR to main.Summary
Prepare the webviews for isolated, deterministic rendering without introducing Component Explorer tooling.
Main was merged with conflicts resolved to preserve upstream issue previews, mergeability polling, checkout updates, and stack refresh behavior. New upstream tests were adapted to explicit hosts.
Validation
npm ci --ignore-scriptsrestored locked dependencies in the owning worktree.npm run test:webviews: normal build succeeded; 81 tests passed.npm run lintandnpm run hygienepassed.Follow-ups
Review scope
The viewport-width hook, its overview/sidebar call sites, and its regression tests are owned entirely by #9048. This PR leaves those production viewport implementations unchanged.