perf(pr): actions update every client from the host's reply instead of re-reads#25761

🟠 High state/pullRequests.ts:595

After an update-branch action, a pending detail read can return old-head metadata labeled with the new headSha, leaving checks, diff counts, and baseComparison stale until another refresh. The post-read reconciliation at line 595 applies patch but never evaluates needsRefresh; evaluate it against the unpatched value and refresh the detail atom when it returns true.

No longer relevant as of 110d8d178bed562080cd5ebaf77dc4b77e3e33bc

Fixed in 110d8d178b. A read started before the action now checks the unpatched response for head-dependent refresh before replaying the result. The regression covers both loaded and pending detail reads and asserts fresh diff counts with exactly two detail reads. It failed on 27ad30f and passes with the fix.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟡 Medium state/pullRequests.ts:431

A delayed hosted result at observedAt 250 is accepted after an equivalent hostless result at 300, so preview and previewCard can regress to older state or draft values. The exact-key lookup at actions.get(key) hides the newer equivalent-reference entry; compare against the newest matching action before accepting a result.

No longer relevant as of 110d8d178bed562080cd5ebaf77dc4b77e3e33bc

Fixed in 110d8d178b. Freshness now uses the newest equivalent reference, and accepted results replace older aliases in the ledger. The regression sends hosted 200, hostless 300, then delayed hosted 250; both preview and previewCard keep the result at 300. It failed on 27ad30f and passes with the fix.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

External image Minor · Functional Correctness · Quick win

Use client time for at, and copy only the fields the list row needs.

at is now result.observedAt, which the server stamps. settlePullRequestOverrides compares at with the client's Date.now() against PULL_REQUEST_OVERRIDE_TRUST_MS. Environments can be remote. If the server clock is more than 60 seconds behind the client, outranked is true on the first list answer. A stale host read, such as one that still shows an open row after a close, then drops the confirmed override immediately.

observedAt already carries the server timestamp for the staleness check in applyPullRequestOverrides. at only needs the local clock, which now supplies.

The ...result spread also copies mergedAt, closedAt, headSha, autoMergeEnabled, and autoMergeMethod into every row that applyPullRequestOverrides merges. PullRequestListEntry does not define these fields. Copy only the fields the override uses.

✅ Addressed in commits b8c6dab to 110d8d1

Fixed in 110d8d178b in both the page override and shared web action-result handler. Expiry uses client time; observedAt stays the server freshness stamp. The shared mapping copies only row override fields. The clock-skew regression failed on 27ad30f and now passes; all 130 focused web tests pass.

@Bil0000 Thanks for the fix. The supplied code sets at from client time, preserves observedAt for freshness checks, and copies only the required override fields through pullRequestResultOverride. This addresses the finding.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

🟠 High state/pullRequests.ts:578

A state-only action patch is reported to onAnswer.detail as a fresh detail read, so the web handler records the inherited detail.checks at the action's newer observedAt. This can replace passing checks observed at 200 with stale pending checks from 100, now marked fresh at 300; the later actionResult handler's recordChecks: false cannot undo it. Keep synthetic metadata patches from advancing checks freshness.

No longer relevant as of 436f608beee4a0838d9a9de88ebcf10511449bd5

Fixed in 436f608bee. Synthetic patches no longer advance checks freshness. When a late read gets newer action metadata, its checks keep the original read timestamp. The extended regression failed on 110d8d178b and now passes for both loaded and late detail reads. Scoped check:changed passes, plus 87 shared-client, 130 web, and 37 mobile focused tests.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High state/pullRequests.ts:849

After update-branch changes a PR head, an already-loaded listStats cache keeps the old additions and deletions until an unrelated refresh. Its fieldRefreshes trigger ignores result-bearing action events, and patching the list does not run onAnswer.list, so mobile's withListedPullRequests updated-date invalidation is skipped too. Add stats invalidation for head-changing action results.

No longer relevant as of 0cdf4d4b613d0e3fc110d8585c6073cfbcfd21f4

Fixed in 0cdf4d4b61. Result events now refresh only stats queries that contain the affected reference, when its confirmed head changes or is unknown. Later results on the same known head keep stats cached. The existing listStats answer handler updates mobile counts. Both loaded/pending regressions failed on 436f608bee and now pass, including fresh counts, an untouched unrelated query, and no extra read for a later ready action. Scoped check:changed and focused client87/web130/mobile37 tests pass.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

External image Minor · Functional Correctness · Quick win

Match stat references without requiring equal account scopes.

If an action reference has expectedAccountId but a mounted listStats reference omits it, sameActionReference returns false. A head change then leaves that PR’s diff stats stale. Stat persistence already keys the row without expectedAccountId. Use a stat-specific match that preserves the PR identity checks but does not require equal account scopes. (raw.githubusercontent.com)

✅ Addressed in commits 3d734d2 to 04c761e

Fixed in 04c761ee7f. Only the stats refresh predicate now ignores account scope; PR/environment/host/project identity checks remain. State patches and write routing still require the original account scope. The added account-pinned case failed on 3d734d267b and passes now, including fresh counts, an untouched unrelated stats query, and no extra read on a later same-head action. All 88 focused shared tests and scoped check:changed pass.

@Bil0000 The supplied code confirms that only the listStats refresh predicate ignores expectedAccountId. Environment and PR identity checks remain intact, and sameActionReference still requires equal account scopes elsewhere. This addresses the finding.

The reported regression test covers the account-pinned case, unrelated-query isolation, and same-head read suppression. I did not rerun the tests.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

🟠 High state/pullRequests.ts:450

A successful reopen from a server with a clock behind the server that handled the close is discarded, so the retained close result keeps the UI showing the pull request as closed. observedAt comes from each routed server's local clock, but this guard compares those timestamps as if they shared one clock; order results by provider freshness or track their clock/source instead.

No longer relevant as of 74224e896b24d395942b8b8c5d4f23da04450edb

Fixed in 7326028875. GitHub updatedAt now takes priority over routed-server observedAt when selecting action metadata. The same comparator covers the shared ledger/read replay, detail/list patches, web overrides/snapshots, and mobile pages. Equal or missing provider dates retain the observation-time fallback, and merged/account guards remain. Shared, web, and mobile clock-regression tests failed before the fix and pass now. Final check:changed passes 165 tests and 31 package type checks; focused client89/web132/mobile38/contracts39 pass.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High state/pullRequests.ts:665

Reopening a PR with an unchanged headSha leaves its comparison-derived baseComparison and update-branch permissions stale, so the branch-update control and behind-base warning remain unavailable. needsRefresh only checks for a changed headSha; include state changes so reopening triggers a detail refresh.

✅ Resolved in d3435a49d1cf3ef48e43e5805624377f49d57d5d

Fixed in d3435a49d1. Detail refresh now covers closed-to-open transitions as well as changed heads. It is limited to reopening, so close/merge still avoid the extra detail read. Loaded and pending same-head reopen regressions failed on 7326028875 and now pass with fresh baseComparison, update-branch permission, and exactly two detail reads. Scoped check:changed passes; focused client91/web132/mobile38 tests pass.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High state/pullRequests.ts:392

When a result-bearing action event arrives while a detail, preview, or list query is pending with no cached value, this branch preserves the prior refresh revision. The pre-action response can then publish and persist stale PR state without a corrective read, because actionPatchedQuery cannot patch a value that is not yet available. Retain the invalidation or restart pending readers before suppressing the event.

No longer relevant as of f2b461d6938a998a8beb107fc8badc5296e7baa4

Fixed in f2b461d693. Readers with no cached value now restart before the result-bearing refresh is suppressed. This is in the shared patch path for detail, preview, and list queries. Loaded values still patch without another read.

The existing late-read regression now covers both cold and warm caches. The cold case failed before the fix with open instead of merged; both cases pass now. All 62 shared PR tests and scoped vp run check:changed HEAD pass, including seven affected package typechecks. Final staged Ponytail review found no open issues.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High state/pullRequests.ts:143

A hostless action reply can patch and persist a different host’s pull request, displaying the wrong title, state, and URL. samePullRequest treats an omitted host as matching any host, so resolve the hostless reference to the project’s repository host before comparing hosts.

No longer relevant as of e45425017746e9468f250ea63a86c739d67c6c08

Fixed in e454250177. A missing host is no longer a wildcard. A reply now updates only readers whose host matches the action's host exactly. A reader without a host keeps its own value until its own read refreshes. Mobile list paging uses the same rule. The existing reader test now mounts a reader without a host and checks that it stays open. That check fails on the old matcher.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟡 Medium server/GitCafePullRequestProvider.ts:1151

ready and draft return mergeability: "unknown", overwriting the previously read conflict status and hiding the conflict/Resolve control even though these actions do not change commits. Json.toChangeRequest supplies this placeholder, and the partial action result forwards it; omit mergeability from the destructuring alongside the line totals.

No longer relevant as of e45425017746e9468f250ea63a86c739d67c6c08

Fixed in e454250177. GitCafe's action reply carries no conflict state, so mergeability is now left out of the summary, like the line totals. The detail and list rows keep the conflict status they last read. The GitCafe close test now checks that mergeability is absent.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

External image Minor · Data Integrity & Integration · Quick win

Keep absent closedAt and mergedAt absent in action replies.

toSummary changes an absent closedAt or mergedAt to null. Action replies now share this mapper. toActionSummary in BitbucketPullRequestProvider.ts deliberately omits both fields because Bitbucket's write reply has no closed_on. The service still sends closedAt: null, mergedAt: null to clients.

The client detail patcher in packages/client-runtime/src/state/pullRequests.ts copies these fields through pickDefined. That helper treats null as defined, so the reply overwrites the cached detail. For example, after a Bitbucket merge, the detail shows state: "merged" with mergedAt: null.

Summary reads keep ?? null. For action replies, omit the fields when the provider value is undefined.

✅ Confirmed as addressed by @Bil0000

Fixed in 548830ad68. Action replies now leave closedAt and mergedAt out when the provider did not send them. Summary reads still map a missing value to null. The merged-reply server test now has no closedAt and checks that the field stays absent. That check fails on the old mapper.

@Bil0000 The supplied code confirms the fix. Action replies omit undefined closedAt and mergedAt fields, while summary reads still map missing values to null. Explicit null values remain unchanged.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Functional Correctness · Quick win

Check stale replies even without observedAt.

PullRequestSummary.observedAt is optional. If an open reply omits it, this guard skips isStalePullRequestReply and applies the open override over a row already marked merged. The predicate already rejects that state regression without needing an observation timestamp. Call it for every reply-derived override.

✅ Confirmed as addressed by @Bil0000

Fixed in 548830ad68. A reply-based override is now marked fromReply and is always checked with isStalePullRequestReply, also when it has no observedAt. The web logic test now checks that an unstamped open reply leaves a merged row merged. That check fails on the old guard.

@Bil0000 The supplied code confirms the fix: every fromReply override passes through isStalePullRequestReply, even without observedAt. This addresses the merged-row regression.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}