Skip to content

fix(server): unticking a GitHub file right after ticking it no longer leaves it viewed - #53

Draft
kvnloo wants to merge 1 commit into
upstream-main-47f2c8d29efrom
fix/host-viewed-files-order
Draft

kvnloo wants to merge 1 commit into
upstream-main-47f2c8d29efrom
fix/host-viewed-files-order

Conversation

@kvnloo

@kvnloo kvnloo commented Nov 2, 2026

Copy link
Copy Markdown
Owner

Problem

On a GitHub pull request, tick a file as viewed and then untick it shortly after. GitHub can end up with the file marked viewed, even though the last thing you did was untick it.

setFilesViewed in apps/server/src/pullRequest/pullRequestViewedFiles.ts already has a per-change-request order gate, inFilesViewedOrder, so that a slow tick cannot overwrite a later untick. Only the environment-backed branch (GitLab and others) goes through it. The host-backed branch (GitHub, viewedFiles: "host") calls the provider write directly.

Why the race happens in the real process:

  • The web client debounces presses for 400 ms (apps/web/src/components/pullRequest/filesViewedStore.ts, flush). Each flush sends pullRequests.setFilesViewed without waiting for the previous flush to settle. Two flushes on one change request can be in flight at the same time, and so can presses from a second client (desktop plus mobile).
  • The WebSocket RPC server uses the Effect RpcServer default concurrency (WS_RPC_SERVER_OPTIONS sets no limit), so both requests run at the same time in one server process. Nothing between ws.ts and the provider orders them: the withRateLimitBackoff wrapper only checks rate limits.
  • GitHubPullRequestApi.setPullRequestFilesViewed first resolves the pull request's node id. That id is cached after the first lookup, so the first press on a change request makes one more GraphQL round trip than later presses. A tick followed by an untick therefore tends to reach GitHub in reverse order.

Change

Route host-backed writes through the existing inFilesViewedOrder(project, number, …) gate, the one the environment branch already uses. Presses on one change request now reach the host in the order they were made. The gate is keyed per change request, so writes on different change requests still run at the same time. One server file changes (+12/−8, including a comment update); tests +84.

Scope and approval

This is a focused fix for an obvious bug, the "very small, focused fix" exception in CONTRIBUTING. The file already states the intended behavior ("Serialize presses so slower tick lookups cannot overwrite later unticks") and implements it for one of its two branches. The fix applies that existing behavior to the other branch. It adds no new mechanism, setting, or product behavior. Duplicate search on 2026-11-02 (open and recently closed PRs and issues: "viewed", "files viewed", "setFilesViewed", "viewed race", …) found no PR or issue for this. Open upstream PR 24151 touches the same file but changes revision baselines for environment marks and does not touch the host branch.

Verification

Base: 47f2c8d29e981c7723467efe137ee19e91df924e (upstream main). Linux, Node 26.12.1, pnpm 12.14.0.

New tests are in apps/server/src/pullRequest/PullRequestService.test.ts and run through the real PullRequestService with a fake GitHub host provider. A Deferred holds the tick's host write, which stands in for the node-id lookup. There are no sleeps. TestClock only advances fibers, following the existing environment-ordering test.

  • host viewed writes on one change request land in the order they were pressed: start tick (held), start untick, release the tick, then assert the host writes are [tick, untick] and that the file reads back as unviewed.
  • a slow host viewed write does not hold up another change request (control): while a write on change request 10 is held, a write on change request 11 completes.
cd apps/server && vp test run src/pullRequest/PullRequestService.test.ts -t "host viewed write"

RED on base (test file only, fix not applied):

 FAIL  src/pullRequest/PullRequestService.test.ts > host viewed writes on one change request land in the order they were pressed
AssertionError: expected [ '10 a.ts false', '10 a.ts true' ] to deeply equal [ '10 a.ts true', '10 a.ts false' ]
      Tests  1 failed | 2 passed | 107 skipped (110)

The untick reached the host first, and the tick then overwrote it.

GREEN with the fix:

 Test Files  1 passed (1)
      Tests  3 passed | 107 skipped (110)

The control passes both before and after the fix, so the gate does not serialize across change requests. The -t "host viewed write" filter also matches the existing partial host viewed writes … test, which is the third test that passes.

Also run:

  • cd apps/server && vp test run src/pullRequest/: 6 files, 141 tests passed.
  • cd apps/server && tsc --noEmit --singleThreaded: exit 0.
  • vp lint --report-unused-disable-directives and vp fmt on the two changed files: clean. The pre-commit hook (fmt, lint, knip:check) passed.

Not checked

  • Not reproduced against live GitHub. The ordering comes from the node-id cache and RPC concurrency described above. The test models the slow first write with a gate.
  • A tick and untick that arrive at two separate T3 servers (two environments writing to the same GitHub account) are outside one process and are not covered.
  • No UI change, so there are no screenshots.

Implemented by Claude Opus 5.5 via the Hermes ACP harness in T3 Code.

… leaves it viewed

Host-backed viewed-file writes went straight to the provider, while
environment-backed ones queue per change request. Two presses on the same
change request run as concurrent RPCs, and the first GitHub press also looks
up the pull request's node id, so a quick tick then untick could land on
GitHub in reverse order and leave the file marked viewed.

Route host writes through the same per-change-request order gate the
environment path already uses. Writes on different change requests stay
concurrent.

Implemented by Claude Opus 5.5 via the Hermes ACP harness in T3 Code.
@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Nov 8, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

1 participant