Recover Rapid Diffs Changes tab when MR preparation is missed

Marc Shawrequested to merge
ms/rapid-diffs-preparing-race into master

Since Rapid Diffs became the default on the Changes tab (f3f3070d), a merge request opened while it is still being prepared can get stuck on the "Building your merge request…" empty state forever. The client queried the MR status first and only then subscribed to preparation updates, and it did nothing when that first status already said "prepared". Now the client emits the reload event when the first status response already says prepared, and re-reads the status once the subscription is acknowledged, so nothing can finish unobserved in either gap.

This fixes the "expected to have file" failure signature of the flaky E2E tests revert_commit_spec.rb and cherry_pick_commit_spec.rb. The other signature they share, "Wait for file tree button to load failed", is a separate bug handled in !255637 (merged).

Detailed context for AI agents

What does this MR do and why?

Rapid Diffs renders the Changes tab server-side. When the MR is in initial_preparation? (merge_status preparing, prepared_at nil), RapidDiffs::MergeRequestAppComponent renders the "Building your merge request… This page will update when the build is complete." empty state. The only recovery is pages/projects/merge_requests/page.js: diffsEventHub.$once(EVT_MR_PREPARED, () => window.location.reload()).

EVT_MR_PREPARED comes from app/assets/javascripts/code_review/signals.js. Previously it queried preparedAt once and, only if null, subscribed to mergeRequestMergeStatusUpdated. Two windows lost the event:

  1. Preparation completed between the server render and the initial status query. The query returned preparedAt set, signals.js only emitted EVT_MR_DIFF_GENERATED (tab counts), and nothing reloaded.
  2. Preparation completed after the query but before the subscription was registered. The broadcast had no subscriber.

Legacy diffs were resilient because the diffs app fetched diffs_metadata.json client-side on tab load and saw the prepared MR regardless of the subscription.

The fix still queries the MR status first, as master did. If that status already says prepared and the page was rendered while preparing, signals.js now emits EVT_MR_PREPARED and EVT_MR_DIFF_GENERATED from that same response instead of doing nothing, which covers window 1. If the status says not prepared, it subscribes to mergeRequestMergeStatusUpdated. GraphqlChannel#subscribed transmits an initial payload (mergeRequestMergeStatusUpdated: null) as soon as the subscription is registered. On that first payload, if the page was rendered while preparing, the client reads the status once more with fetchPolicy: 'network-only'; if the re-read says prepared, it emits both events from that data. This covers window 2, where preparation finished before the subscription was registered and the broadcast had no subscriber. The re-read happens at most once. If the subscription later reports preparedAt directly, it emits EVT_MR_PREPARED and does a network-only refetch for EVT_MR_DIFF_GENERATED, as master did. A prepared guard makes sure only the first path to fire acts, and the subscription is unsubscribed once the MR is known to be prepared. Pages not rendered while preparing keep master's behaviour and never re-read on the acknowledgement.

This shape was chosen over gating the status query on the subscription acknowledgement. Nothing waits on the websocket, so there is no timeout: a cable that never connects, or a subscription answering with an error payload (ActionCableLink routes top level errors to observer.error, and only a next handler is passed here), never acknowledges, and gating would have held the only rescue request for 5 seconds. The cost is one extra status query on a page rendered while preparing. This shape was proposed in review: !255351 (comment 3843687153)

Not covered by this MR

The two flaky tests share a second failure signature, "Wait for file tree button to load failed after 60 seconds", which is a different bug. On the Overview tab, clicking Changes while the MR has no diff triggers a full navigation to /diffs. If the prepared broadcast lands during that navigation, window.location.reload() in page.js reloads the Overview URL and cancels the navigation, and the QA click_diffs_tab step then keeps refreshing the Overview URL for 60 seconds. Reproduced on GDK on both the parent commit and this branch. The fix is to only reload when the Changes tab is the current tab, tracked in !255637 (merged).

Changes

  • _page.html.haml: the .js-changes-tab-count span gains data-preparing="true|false" (from initial_preparation?, the same predicate that gates the empty state) so the client knows whether the page was rendered while the MR was preparing. The span already carries the MR GID.
  • code_review/signals.js: always query the MR status first (mergeRequestId), as master did. If that query already reports preparedAt and the page was rendered while preparing (data-preparing="true" on .js-changes-tab-count), emit EVT_MR_PREPARED and EVT_MR_DIFF_GENERATED using that same response, instead of doing nothing. Otherwise, if the MR is not yet prepared, subscribe to mergeRequestMergeStatusUpdated. The subscription's first payload is the server's acknowledgement (mergeRequestMergeStatusUpdated: null); if the page was rendered while preparing, that first payload triggers one re-read of the status with fetchPolicy: 'network-only', and if the re-read says prepared, both events are emitted from it. If the subscription later reports preparedAt directly, emit EVT_MR_PREPARED and do a network-only refetch for EVT_MR_DIFF_GENERATED, since the subscription payload carries no counts. A prepared guard makes sure only the first path to fire acts and unsubscribes once the MR is known to be prepared. The re-read on acknowledgement happens at most once, and pages not rendered while preparing never trigger it. Payloads without data are tolerated.

Alternatives considered

  • Poll preparedAt as a fallback (first iteration of this MR). Works but papers over the ordering bug and adds up to 24 requests per stuck page.
  • Fix only in the QA spec by waiting for the MR to be prepared via API. Real users hit the same stuck state.
  • Render the Rapid Diffs list lazily on tab click like legacy. Much larger change.

Reproduction

spec/features/merge_request/rapid_diffs/preparing_merge_request_spec.rb covers two cases. The first renders the Changes tab for a preparing MR, then runs MergeRequests::AfterCreateService to finish preparation, and asserts the subscription recovers the page.

The second is written to be deterministic rather than relying on timing. It stubs GitlabSchema.execute to hold the client's mergeRequestId status query until prepared_at is set, then sets prepared_at with update_columns (no broadcast is sent), and asserts that the "Building your merge request" empty state disappears and diff-file elements are present. The assertion is on the empty state disappearing, not on diff-file presence alone, because Rapid Diffs streams an existing diff behind the empty state. This example fails against the parent commit (the empty state never disappears) and passes with the fix.

The earlier version of that second example passed against the parent commit 3 out of 3 runs on GDK, because AfterCreateService happened to finish after the client's status query, so the subscription rescued it. That made the test non-reproducing for the actual bug, which is why it was rewritten to hold the query deterministically.

The first failure in the flaky test issues is Sep 9, 2026, the day f3f3070d merged. The E2E failure log also shows a 404 on diffs_metadata.json, confirming the MR had no merge_request_diff when the page rendered.

References

Screenshots or screen recordings

Before After
live_base_race live_fix_race
Stuck on "Building your merge request…" with an empty file tree, no recovery (90s+ after preparation finished) Page reloads as soon as the status response arrives and shows the diff

How to set up and validate locally

  1. Make sure rapid_diffs_default_on_mr_show is enabled (default).
  2. Stop Sidekiq so MR preparation does not run: gdk stop rails-background-jobs.
  3. Create an MR via the UI or API and open its Changes tab. It shows "Building your merge request…".
  4. Start Sidekiq again: gdk start rails-background-jobs.
  5. Without the fix the page never updates if preparation completed before the client asked for the status. With the fix it reloads and shows the diff either from the status response or from the subscription, whichever happens first.

Automated verification: jest spec/frontend/code_review/signals_spec.js (27 tests) and spec/frontend/pages/projects/merge_requests/page_spec.js pass. bundle exec rspec spec/features/merge_request/rapid_diffs/preparing_merge_request_spec.rb passes both examples. With the two product files swapped back to the parent commit, the second example fails as expected.

Live verification on GDK

Verified on 2026-09-15 using headless Chrome driven over CDP, with Sidekiq stopped, an MR created via the API, and Fetch.requestPaused holding the client's mergeRequestId status request until Sidekiq had finished preparing the MR.

  • Parent commit 70c3fc35: the page stayed on "Building your merge request…" 15 seconds after the held status response came back "prepared". No reload happened.

The reviewer separately ran both windows on GDK against this proposed shape (!255351 (comment 3843687153)), holding the status query with Fetch.requestPaused and setting prepared_at with update_columns so no broadcast is sent: window 1 reloaded 0.9 seconds after the held response, window 2 reloaded 1.4 seconds after the second status query.

Re-verified on 2026-09-16 with this revision on GDK, anonymous headless Chrome over CDP against a public project MR whose diff already existed, with prepared_at reset to null and merge_status to preparing first. Fetch.requestPaused held the mergeRequestId query, prepared_at was set with a direct SQL update so no broadcast was sent, and the request was then released. Window 1 (first query held): the page reloaded and rendered diff-file 4.6 seconds after release. Window 2 (first query answered preparing, second query held): the client sent the second mergeRequestId query about 1.5 seconds after the page loaded, and the page reloaded and rendered diff-file 0.7 seconds after release. One run per window.

MR acceptance checklist

Evaluated against the MR acceptance checklist. No database, API, or security surface changes. The subscription is only opened on pages rendered while the MR was preparing, and is closed as soon as the MR is known to be prepared.

Edited by Marc Shaw

Merge request reports