Fix mergeability polling in pull request overview - #8954
Fix mergeability polling in pull request overview#8954Viktor Sharha (vsharha) wants to merge 2 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
🔵 Needs a closer look
Add a component test covering the unknown mergeability polling transition.
Pull request overview
Fixes mergeability polling by unwrapping the API response and sharing its response type across the model and webview.
Changes:
- Corrects polling state updates.
- Adds
PullRequestMergeabilityResult. - Applies the shared type across relevant APIs.
File summaries
| File | Summary |
|---|---|
webviews/components/merge.tsx |
Unwraps mergeability; needs coverage for the unknown-state polling transition. |
webviews/common/context.tsx |
Types the mergeability response. |
src/github/pullRequestModel.ts |
Uses the shared response type. |
src/github/interface.ts |
Defines the shared result interface. |
Review details
Suppressed comments (1)
webviews/components/merge.tsx:196
- This regression fix has no test covering the
Unknownpolling path. The existing webview tests do not renderMergeStatusAndActionsor verify that a{ mergeability, conflicts }response updates the displayed state, so this response-shape bug could recur without failing the listed checks. Add a component test with a mockedcheckMergeabilityresponse and timer advancement that asserts the checking state transitions.
const { mergeability: newMergeability } = await checkMergeability();
setMergeability(newMergeability);
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The regression test is not discovered or executed by the configured test runner.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| it('updates an unknown mergeability state from the polling response', async function () { | ||
| const pr = new PullRequestBuilder() | ||
| .mergeable(PullRequestMergeability.Unknown) | ||
| .hasWritePermission(false) | ||
| .build(); | ||
| const context = new PRContext(pr); | ||
| const checkMergeability = sinon.stub(context, 'checkMergeability').resolves({ | ||
| mergeability: PullRequestMergeability.Mergeable, | ||
| }); |
There was a problem hiding this comment.
The initial Copilot review specifically requested a component test for MergeStatusAndActions to match the existing webview tests.
All React component tests in this repository (app.test.tsx, overview.test.tsx, and reviewSummary.test.tsx) are in webviews/editorWebview/test/. src/test/ is configured only for extension-host tests under tsconfig.test.json without JSX support, so component tests cannot be placed there.
Setting up a dedicated test runner for webview component tests is broader infrastructure work that is outside the scope of this bug fix.
Fixes #8953
Summary
When GitHub initially returns an unknown mergeability state, the polling request returns an object containing
mergeabilityandconflicts. The webview previously stored that entire object as its mergeability state, so polling stopped and the status remained "Checking if this branch can be merged..." indefinitely.Testing
npm run compile:nodenpm run lintnpm run hygienegit diff --check