Skip to content

perf(pr): reduce quota use when refreshing workspace checks - #12442

Merged
juliusmarminge merged 9 commits into
pingdotgg:t3code/codex-turn-mappingfrom
Bil0000:fix/pr-check-refresh-quota
Sep 21, 2026
Merged

juliusmarminge merged 9 commits into
pingdotgg:t3code/codex-turn-mappingfrom
Bil0000:fix/pr-check-refresh-quota

Conversation

@Bil0000

@Bil0000 Bil0000 commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Workspace PR checks currently trigger a full GitHub read every 45 seconds, even when all checks have finished. This PR reuses unchanged checks through authenticated REST ETags, checks every page, keeps caches separate by account, and preserves fork workflow approval and rate-limit handling. A full read every five minutes bounds stale data if the REST and GraphQL views disagree.

Pending, action-required, or missing checks refresh every 45 seconds; settled checks refresh every 60 seconds. Hidden and idle views pause polling. Real pointer, key, or wheel input resumes idle views; focus and visibility events alone do not. T3 pushes notify the project immediately, including draft worktrees.

Verified with 388 focused tests, web and server type checks with Effect diagnostics enabled, scoped lint, and formatting. Regression tests cover idle focus/visibility and clearing old cache values after an incomplete forced refresh. Both regressions failed before their fixes. No visual layout changed.

Synced the current base and fixed two CI regressions in ProjectionStore: reuse the existing run-ID JSON encoder, and load shared root checkpoint scopes by ID so later queued turns cannot hide them from earlier turns. The checkpoint regression failed before the fix; both storage variants, all 65 provider-switch tests, and all six affected queued-turn replay cases pass.

Conditional requests still create network traffic and remain subject to secondary limits. External changes are normally seen on the next 45–60 second poll; the five-minute full read is a safety fallback.

Stacked on #2829 (t3code/codex-turn-mapping).

Model: GPT-6. Harness: OpenAI Codex.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Sep 18, 2026
Comment thread packages/client-runtime/src/state/vcsAction.ts
Comment thread apps/server/src/pullRequest/gitHubConditionalChecks.ts Outdated
Comment thread apps/server/src/pullRequest/gitHubConditionalChecks.ts Outdated
@macroscopeapp

This comment has been minimized.

@macroscopeapp

macroscopeapp Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR introduces a new authenticated GitHub conditional-check caching subsystem and changes existing polling, idle-refresh, push invalidation, and orchestration behavior across server and web paths. The altered product refresh defaults and broad runtime surface warrant human review.

You can add or adjust custom eligibility rules. Learn more.

@Bil0000

Bil0000 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

The latest code and Effect reviews are clear; all three findings are fixed and resolved.

CI is blocked by the existing queued_cancelled_while_active/codex replay fixture in Test Server 3: release_replay_gate:turn/completed:reached=false. That shard passed on the previous push, and the exact failing case passes locally on the latest commit:

vp test run apps/server/src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts -t 'queued_cancelled_while_active/codex'

Please rerun the failed job. GitHub rejected my rerun request with HTTP 403, requiring repository admin rights.

@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch 8 times, most recently from 5ff0a93 to fd8ea2f Compare September 19, 2026 04:23
@Bil0000
Bil0000 force-pushed the fix/pr-check-refresh-quota branch from 11ddbab to 435b6e3 Compare September 19, 2026 07:29
@Bil0000

Bil0000 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the current t3code/codex-turn-mapping tip (f180e95).

The base branch was force-pushed, which stranded this PR: GitHub compared it against a rewritten base, so it reported CONFLICTING with ~1400 changed files that were never part of this change. This PR's own commits were replanted onto the new base tip; every commit applied with no conflicts, and the diff against the new base is identical to the diff before the rebase. No behavior was changed as part of the rebase.

@juliusmarminge juliusmarminge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 435b6e3. The revalidator itself is solid and the tests cover the paths I cared about (304 reuse, later pages, fork approvals, head change, account separation, forced 5-minute read). One regression in the shared hook needs fixing before this lands.

Blocking

useLiveRefresh: arrival now counts as interaction, which defeats the idle gate for every live view.

useLiveRefresh.ts:149 adds if (visible()) lastInteractedAt = now; inside onArrival. onArrival runs on focus and visibilitychange, not only on real input. A window left visible on a second monitor gets a focus event every time the person alt-tabs on their main monitor, so lastInteractedAt keeps advancing and shouldRefreshOnInterval never sees the six-minute idle threshold. That is exactly the case the hook's own doc comment (lines 24–30) says it exists to prevent, and it applies to the PR list, the PR detail panel, and the workspace row, not just checks.

Fix: remove that line from onArrival and bump lastInteractedAt only inside the watchInteraction resume callback, where a pointer/key/wheel event is the cause. The existing test resumes via pointerdown so it still passes; please add a case that fires window.focus after six idle minutes and asserts no read.

Non-blocking

  • usePullRequestChecksRefresh carries a waitingSince state machine plus headSha/updatedAt props (and the new headSha field on PullRequestChecks) to give a fresh push two minutes at 45s instead of 60s. That is a lot of plumbing for a 15-second difference. checks.some(pending) || checks.length === 0 ? 45_000 : 60_000 would drop all of it and the contract change. Your call; I would take the simpler one.
  • mergeable_state: unstable on this PR is only the Vercel marketing status; this diff does not touch marketing, so ignore it.

@Bil0000

Bil0000 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Handled both review notes in b2aafe7489 and 118507a78d.

  • Removed the idle-clock reset from arrival. Idle focus/visibility events now perform no read and leave polling paused. Only the existing pointer/key/wheel handler updates the interaction clock and resumes the view. Added regression cases for focus and visibility after six idle minutes, then verified that pointer input resumes reads.
  • Took the simpler polling rule: pending or empty checks use 45 seconds, settled checks use 60 seconds. Removed the waiting state, head-SHA/update-time props, and the added PullRequestChecks.headSha contract field.

263 focused tests and web/server type checks pass. Scoped lint and formatting pass, with one existing ref-during-render warning in useLiveRefresh. The new idle tests fail on the previous revision. The PR body now reflects the simpler behavior. Watching the new CI and doing the full post-fix audit next.

@Bil0000

Bil0000 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

The full post-fix audit found one further bug: after a forced five-minute read could not confirm workflow approvals, the cache kept the older result and marked it fresh for another five minutes. The next unchanged poll could replace the unknown approval state with an old success. d7efcb604d now clears that value; the regression test fails before the fix and passes after it.

CI also caught TS377026 in the updated base branch at ProjectionStore.getTurnStartHistory. Synced the base and replaced that JSON.stringify call with the existing encodeIdList. Existing SQLite and memory tests cover omitted, empty, matching, and nonmatching run IDs.

388 focused tests, web/server type checks with Effect diagnostics enabled, scoped lint, and formatting pass. The full PR audit found no further code blockers after these fixes. Watching the latest CI and reviews. A follow-up review from Julius is still needed; the request-review API returned HTTP 404.

Comment thread apps/web/src/components/chat/ThreadDetailsPrRow.tsx
Comment thread apps/server/src/git/refreshPushedPullRequests.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Effect service convention review found one blocking issue. See the inline review comment for the expected namespace-import fix.

Posted via Macroscope — Effect Service Conventions

@Bil0000

Bil0000 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

The next CI pass exposed a base-branch queued-turn regression. Later queued turns reuse the root checkpoint scope and replace its node metadata; the startup query then lost that scope for earlier turns. fb9b65c612 now looks it up through checkpointScopeId.

The SQLite regression failed before the fix. Both storage variants, all 65 provider-switch tests, and all six affected replay cases pass. Server types, scoped lint, formatting, and ponytail-review pass. Both new Macroscope notes are fixed and resolved. Watching fresh CI and reviews on this commit.

@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch 2 times, most recently from a1f8051 to 0337dd6 Compare September 21, 2026 05:40
@juliusmarminge
juliusmarminge force-pushed the fix/pr-check-refresh-quota branch from fb9b65c to 1864d77 Compare September 21, 2026 18:02
@juliusmarminge

Copy link
Copy Markdown
Member

Rebased onto the current t3code/codex-turn-mapping tip (0e68214) as a clean cherry-pick of the nine feature commits; no conflicts. Dropped the two base-branch commits (24f8c05, fb9b65c) because the base now carries its own versions of both fixes (encodeIdList in getTurnStartHistory, and the checkpoint-scope lookup via sql.in(nodes…checkpointScopeId)). Feature diff against the base is unchanged. New head is 1864d77.

@juliusmarminge

Copy link
Copy Markdown
Member

CI on 1864d77: everything green except Test Server 3, which fails five cases in SteeringCompletion.integration.test.ts (expected 1 to equal 2). That is a base-branch regression, not this PR: the same five cases fail on the bare t3code/codex-turn-mapping tip 0e68214 and pass on its parent 7bc0712. This PR does not touch steering or mailbox code. I'll fix it on the base; no action needed here.

@juliusmarminge
juliusmarminge merged commit c1bf586 into pingdotgg:t3code/codex-turn-mapping Sep 21, 2026
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants