Skip to content

Commit 8eb624f

Browse files
mchwangmchwang
committed
Keep confirmed merge queue progress
1 parent 7883fc3 commit 8eb624f

3 files changed

Lines changed: 91 additions & 3 deletions

File tree

‎docs/implementation/planning-screen.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que
1010

1111
## Import contract
1212

13-
The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. A terminal full response remains authoritative over either active or matching-terminal poll state so its verified terminal result, blockers and retry readiness are not discarded. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens.
13+
The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. A terminal full response remains authoritative over either active or matching-terminal poll state so its verified terminal result, blockers and retry readiness are not discarded. A queued full response likewise wins over a stale submitting observation. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens.
1414

1515
The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged.
1616

1717
## Acceptance
1818

19-
`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, terminal full-response authority over matching terminal and active polls, submitting-to-queued progress, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision.
19+
`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, terminal full-response authority over matching terminal and active polls, submitting-to-queued progress in both response orders, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision.
2020

2121
G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate.

‎test/browser/plans.spec.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -933,6 +933,92 @@ test('lets a terminal full response replace an active poll for the same merge at
933933
expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' });
934934
});
935935

936+
test('lets a queued full response replace a stale submitting poll for the same merge attempt', async ({ page }) => {
937+
test.slow();
938+
const config = { ...app.service.config, demo: false };
939+
await app.close();
940+
chmodSync(join(config.repository, 'run.sh'), 0o644);
941+
fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope');
942+
let appRef: typeof app;
943+
const gateway: MergeGateway & MergeQueueGateway = {
944+
inspect: async () => {
945+
const snapshot = appRef.service.load().snapshot;
946+
return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true,
947+
atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false };
948+
},
949+
queueWatermark: async () => null,
950+
merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }),
951+
inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 3,
952+
enqueuedAt: '2026-10-08T08:00:00Z', queueHead: head }),
953+
};
954+
app = appRef = await startServer(config, 0, undefined, gateway);
955+
let review = app.service.load();
956+
for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous'))
957+
review = app.service.act({ action: 'accept', key: segment.key, token: review.token });
958+
for (const item of review.items)
959+
review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token });
960+
const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID());
961+
962+
await page.goto(app.url);
963+
await expect(page.getByRole('button', { name: 'Submitting…', exact: true })).toBeDisabled();
964+
app.service.store.queueMergeAttempt(config.identity, attempt.id, 'https://github.com/example/repo/pull/7');
965+
app.service.store.observeQueuedMerge(config.identity, attempt.id, { entryId: 'MQE_1', phase: 'QUEUED', position: 3 });
966+
967+
let releaseRefresh!: () => void, refreshArrived!: () => void;
968+
const heldRefresh = new Promise<void>(resolve => { releaseRefresh = resolve; });
969+
const capturedRefresh = new Promise<void>(resolve => { refreshArrived = resolve; });
970+
await page.route('**/api/review', async route => {
971+
const response = await route.fetch();
972+
const refreshed = await response.json();
973+
expect(refreshed.merge.queue).toMatchObject({
974+
actionId: attempt.actionId, reviewedHead: review.snapshot.head, state: 'queued', phase: 'QUEUED', position: 3,
975+
});
976+
refreshArrived();
977+
await heldRefresh;
978+
await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) });
979+
});
980+
await page.getByRole('link', { name: 'Plans', exact: true }).click();
981+
await page.locator('#plans-refresh').click();
982+
await capturedRefresh;
983+
984+
let polls = 0, releaseLaterPolls!: () => void;
985+
const heldLaterPolls = new Promise<void>(resolve => { releaseLaterPolls = resolve; });
986+
await page.route('**/api/merge', async route => {
987+
if (polls++ > 0) {
988+
await heldLaterPolls;
989+
await route.continue();
990+
return;
991+
}
992+
await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: {
993+
kind: 'queue', state: 'submitting', actionId: attempt.actionId, reviewedHead: review.snapshot.head,
994+
url: null, reason: null, phase: null, position: null, occurredAt: '2026-10-08T08:05:00Z', retryable: false,
995+
observationError: 'The older queue inspection failed.',
996+
} }) });
997+
});
998+
const stalePoll = page.waitForResponse(response => response.url().endsWith('/api/merge'));
999+
await page.getByRole('link', { name: 'Review', exact: true }).click();
1000+
await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click();
1001+
await stalePoll;
1002+
await page.evaluate(() => new Promise<void>(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))));
1003+
await expect(page.locator('#banner')).toContainText('The older queue inspection failed.');
1004+
1005+
try {
1006+
const refreshResponse = page.waitForResponse(response => response.url().endsWith('/api/review'));
1007+
releaseRefresh();
1008+
await (await refreshResponse).finished();
1009+
await page.evaluate(() => new Promise<void>(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))));
1010+
await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled();
1011+
await expect(page.locator('#banner')).not.toContainText('The older queue inspection failed.');
1012+
await page.getByRole('button', { name: 'Merge status', exact: true }).click();
1013+
await expect(page.locator('#dialog-body')).toContainText('queued');
1014+
await expect(page.locator('#dialog-body')).toContainText('position 3');
1015+
await expect(page.locator('#dialog-body')).not.toContainText('The older queue inspection failed.');
1016+
expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'queued', position: 3 });
1017+
} finally {
1018+
releaseLaterPolls();
1019+
}
1020+
});
1021+
9361022
test('does not let an older review action response replace a newer queued observation', async ({ page }) => {
9371023
test.slow();
9381024
const config = { ...app.service.config, demo: false };

‎web/public/app.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -826,11 +826,13 @@ function preserveSettledQuestionAnswers(updated) {
826826
function newerMergeQueue(updated, observationOwner) {
827827
const newer = data?.merge?.queue, older = updated.merge?.queue;
828828
const fullResponseIsTerminal = ["merged", "removed", "failed"].includes(older?.state);
829+
const pollRegressesQueued = older?.state === "queued" && newer?.state === "submitting";
829830
return observationOwner !== mergeObservationGeneration &&
830831
newer?.actionId &&
831832
newer.actionId === older?.actionId &&
832833
newer.reviewedHead === older.reviewedHead &&
833-
!fullResponseIsTerminal
834+
!fullResponseIsTerminal &&
835+
!pollRegressesQueued
834836
? newer
835837
: null;
836838
}

0 commit comments

Comments
 (0)