From af6cef2f9b9f77d47e4580084328014d78f49122 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Sun, 25 Oct 2026 14:09:14 -0700 Subject: [PATCH 1/4] fix(web): pull request checks stay current after a checks refresh Pull request summaries shared between the sidebar, right panel tabs, the pull requests page and the detail panel were published from components: two layout effects in state/pullRequests.ts copied list rows and the caller's current detail into the shared summary after each commit. The detail path ordered its record by the detail's server read time, so a newer checks read tied with the summary the detail had already placed and was dropped. The detail panel's checks rollup and merge control kept the previous checks state until the detail itself was reread. Environment queries now take an onAnswer handler that runs where each server answer lands, before readers see it (cached and provisional values are not answers). The web pull request atoms use it for list, detail, checks and linked-summary answers. Detail and checks answers are combined per pull request, and a newer checks read is ordered by its own read time. The detail snapshot for cold starts is written from fresh detail answers too. Effects removed: both layout effects in state/pullRequests.ts and the snapshot-writing effect in usePullRequestDetail.ts. None kept. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/RightPanelTabs.tsx | 2 +- .../src/components/ThreadStatusIndicators.tsx | 10 +- .../pullRequest/PullRequestsPage.tsx | 1 - .../pullRequest/usePullRequestDetail.ts | 31 +--- apps/web/src/state/pullRequests.test.ts | 56 +++++- apps/web/src/state/pullRequests.ts | 169 +++++++++++++----- .../client-runtime/src/state/pullRequests.ts | 15 ++ .../client-runtime/src/state/runtime.test.ts | 53 ++++++ packages/client-runtime/src/state/runtime.ts | 27 ++- 9 files changed, 276 insertions(+), 88 deletions(-) diff --git a/apps/web/src/components/RightPanelTabs.tsx b/apps/web/src/components/RightPanelTabs.tsx index de548692f88c2..59bc657242806 100644 --- a/apps/web/src/components/RightPanelTabs.tsx +++ b/apps/web/src/components/RightPanelTabs.tsx @@ -735,7 +735,7 @@ function PullRequestSurfaceIcon({ repository: surface.repository, number: surface.number, }; - const sharedSummary = useSharedPullRequestSummary(resolvedEnvironmentId, reference, null); + const sharedSummary = useSharedPullRequestSummary(resolvedEnvironmentId, reference); const status = linkedSnapshot ?? newestPullRequestSummary(detail, sharedSummary) ?? seed ?? null; if (status === null) { return ; diff --git a/apps/web/src/components/ThreadStatusIndicators.tsx b/apps/web/src/components/ThreadStatusIndicators.tsx index 840573a2c0774..15fe062a960f9 100644 --- a/apps/web/src/components/ThreadStatusIndicators.tsx +++ b/apps/web/src/components/ThreadStatusIndicators.tsx @@ -95,17 +95,13 @@ export function useLinkedThreadPullRequest( current === null ? ((!supportsLinks ? linkedPullRequest : null) ?? branchPullRequest) : null; // Preserve reference identity so sidebar renders do not touch the summary cache. const reference = fallback == null ? null : linkedPullRequestReference(fallback); - const queried = useEnvironmentQuery( + // Mounted for its answers, which land in the shared summary read below. + useEnvironmentQuery( !enabled || environmentId === null || reference === null ? null : linkedPullRequestDetailAtom({ environmentId, input: reference }), ); - const detail = useSharedPullRequestSummary( - environmentId, - reference, - queried.data, - queried.dataUpdatedAt, - ); + const detail = useSharedPullRequestSummary(environmentId, reference); if (current !== null) return currentStatus; return detail === null diff --git a/apps/web/src/components/pullRequest/PullRequestsPage.tsx b/apps/web/src/components/pullRequest/PullRequestsPage.tsx index 57ae57924c998..65765be00d08a 100644 --- a/apps/web/src/components/pullRequest/PullRequestsPage.tsx +++ b/apps/web/src/components/pullRequest/PullRequestsPage.tsx @@ -205,7 +205,6 @@ export function PullRequestsPage() { ...renderedPullRequestSurface, projectId: renderedPullRequestSurface.projectId as ProjectId, }, - null, ); const updateSearch = (patch: { [Key in keyof PullRequestsSearch]?: PullRequestsSearch[Key] | undefined; diff --git a/apps/web/src/components/pullRequest/usePullRequestDetail.ts b/apps/web/src/components/pullRequest/usePullRequestDetail.ts index a721129fbb08e..7451ee38c5fee 100644 --- a/apps/web/src/components/pullRequest/usePullRequestDetail.ts +++ b/apps/web/src/components/pullRequest/usePullRequestDetail.ts @@ -2,11 +2,10 @@ import { resolvePullRequestDetailChecks } from "@t3tools/client-runtime/pull-req import { AuthSourceControlWriteScope, type EnvironmentId, - type PullRequestDetail, type PullRequestListEntry, type PullRequestRef, } from "@t3tools/contracts"; -import { useEffect, useEffectEvent, useState } from "react"; +import { useState } from "react"; import { usePullRequestChecksRefresh } from "~/hooks/usePullRequestChecksRefresh"; import { useServerConfigs } from "~/state/entities"; @@ -21,18 +20,12 @@ import { import { readPullRequestDetailSnapshot, resolveDisplayedPullRequestDetail, - writePullRequestDetailSnapshot, } from "./pullRequestDetail.logic"; -import { pullRequestChecksState } from "./pullRequestPresentation"; export function pullRequestRefKey(reference: PullRequestRef) { return `${reference.projectId}:${reference.host ?? ""}:${reference.repository}#${reference.number}`; } -function pullRequestDetailSummary(data: PullRequestDetail | null) { - return data === null ? null : { ...data, checksState: pullRequestChecksState(data.checks) }; -} - /** * Loads a pull request's detail and activity and merges them, with the snapshot cache, the shared * summary and the list row, into the detail the panel shows. @@ -95,24 +88,7 @@ export function usePullRequestDetail( } else if (detailQuery.data !== null && detailQuery.data !== cachedDetail) { setCachedDetail(detailQuery.data); } - const saveDetailSnapshot = useEffectEvent((data: PullRequestDetail) => { - writePullRequestDetailSnapshot( - typeof window === "undefined" ? undefined : window.localStorage, - environmentId, - reference, - data, - ); - }); - useEffect(() => { - if (detailQuery.data !== null) saveDetailSnapshot(detailQuery.data); - }, [detailQuery.data]); - const detailSummary = pullRequestDetailSummary(liveDetail); - const observedSummary = useSharedPullRequestSummary( - environmentId, - reference, - detailSummary, - Math.max(detailQuery.dataUpdatedAt, checksQuery.dataUpdatedAt), - ); + const observedSummary = useSharedPullRequestSummary(environmentId, reference); const activity = activityQuery.data; const resolvedCoreDetail = resolveDisplayedPullRequestDetail({ live: liveDetail, @@ -122,8 +98,7 @@ export function usePullRequestDetail( // Every hook runs above this line: a hook between the merge and the panel's plain readers of // `detail` would cost it its cache scope. const listSummary = listEntry === null ? null : pullRequestListEntryToSummary(listEntry); - // The list row is also published to the shared cache, but only after this commit's layout - // effects run, so it is compared directly rather than trusted to be there already. + // The row can come from the page's stored snapshot, which no answer has published yet. const sharedSummary = newestPullRequestSummary( resolvedCoreDetail, newestPullRequestSummary(observedSummary, listSummary), diff --git a/apps/web/src/state/pullRequests.test.ts b/apps/web/src/state/pullRequests.test.ts index 9bdd84c96c2c4..140b1a78315eb 100644 --- a/apps/web/src/state/pullRequests.test.ts +++ b/apps/web/src/state/pullRequests.test.ts @@ -1,8 +1,14 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; -import { ProjectId, type PullRequestSummary } from "@t3tools/contracts"; +import { + ProjectId, + type PullRequestCheck, + type PullRequestDetail, + type PullRequestSummary, +} from "@t3tools/contracts"; import { describe, expect, it } from "vite-plus/test"; import { + livePullRequestObservation, newestPullRequestObservation, newestPullRequestSummary, threadPullRequestLinkToSummary, @@ -114,3 +120,51 @@ describe("pull request summary cache", () => { ).toBeNull(); }); }); + +describe("live pull request answers", () => { + const failing: PullRequestCheck = { + name: "build", + status: "failure", + url: null, + description: null, + }; + const detail = { ...summary(), checks: [] } as unknown as PullRequestDetail; + + it("lets whichever of detail and checks answered last decide the checks", () => { + const checks = { value: { state: "open" as const, checks: [failing] }, at: 200 }; + expect(livePullRequestObservation({ detail: { value: detail, at: 100 }, checks })).toEqual({ + summary: { ...detail, checks: [failing], checksState: "failing" }, + observedAt: 200, + }); + expect( + livePullRequestObservation({ detail: { value: detail, at: 300 }, checks }), + ).toMatchObject({ summary: { checks: [], checksState: null }, observedAt: 300 }); + }); + + it("lets a newer checks read replace the detail's checks while stale list reads stay out", () => { + const pending: PullRequestCheck = { ...failing, status: "pending" }; + const passing: PullRequestCheck = { ...failing, status: "success" }; + const read = { ...detail, observedAt: 100, checks: [pending] } as PullRequestDetail; + const fromDetail = livePullRequestObservation({ + detail: { value: read, at: 1_000 }, + checks: null, + }); + const fromChecks = livePullRequestObservation({ + detail: { value: read, at: 1_000 }, + checks: { value: { observedAt: 300, state: "open", checks: [passing] }, at: 2_000 }, + }); + const accepted = newestPullRequestObservation(fromDetail, fromChecks); + expect(accepted?.summary.checksState).toBe("passing"); + const staleList = observed(summary({ checksState: "pending", observedAt: 200 }), 3_000); + expect(newestPullRequestObservation(accepted, staleList)).toBe(accepted); + }); + + it("waits for a detail answer before observing checks", () => { + expect( + livePullRequestObservation({ + detail: null, + checks: { value: { state: "open", checks: [failing] }, at: 1 }, + }), + ).toBeNull(); + }); +}); diff --git a/apps/web/src/state/pullRequests.ts b/apps/web/src/state/pullRequests.ts index 7e39460dbc758..d151825451024 100644 --- a/apps/web/src/state/pullRequests.ts +++ b/apps/web/src/state/pullRequests.ts @@ -8,6 +8,8 @@ import { import type { EnvironmentId, ProjectId, + PullRequestChecks, + PullRequestDetail, PullRequestListInput, PullRequestFilterOptionsInput, PullRequestListStatsInput, @@ -20,9 +22,11 @@ import { threadPullRequestKeyOf, visibleThreadPullRequests, } from "@t3tools/client-runtime/thread-pull-requests"; +import { resolvePullRequestDetailChecks } from "@t3tools/client-runtime/pull-request-checks"; +import * as Clock from "effect/Clock"; +import * as Effect from "effect/Effect"; import * as Option from "effect/Option"; -import { AsyncResult, Atom } from "effect/reactivity"; -import { useLayoutEffect } from "react"; +import { AsyncResult, Atom, type AtomRegistry } from "effect/reactivity"; import { connectionAtomRuntime } from "../connection/runtime"; import { appAtomRegistry } from "../rpc/atomRegistry"; @@ -31,15 +35,11 @@ import { type EnvironmentPullRequestStat, type MergedPullRequestList, } from "../components/pullRequest/pullRequestList.logic"; +import { writePullRequestDetailSnapshot } from "../components/pullRequest/pullRequestDetail.logic"; +import { pullRequestChecksState } from "../components/pullRequest/pullRequestPresentation"; import { formatEnvironmentQueryError } from "./query"; import { environmentThreadShells } from "./threads"; -export const pullRequestEnvironment = createPullRequestEnvironmentAtoms(connectionAtomRuntime); -export const linkedPullRequestDetailAtom = createLinkedPullRequestSummaryAtomFamily( - connectionAtomRuntime, - pullRequestEnvironment.refreshes, -); - export interface ObservedPullRequestSummary { readonly summary: PullRequestSummary; /** Client arrival time, the only ordering older servers leave us for same-dated snapshots. */ @@ -137,17 +137,125 @@ export function newestPullRequestObservation( } function observePullRequestSummary( + registry: AtomRegistry.AtomRegistry, key: string, summary: PullRequestSummary, observedAt: number, ): void { - const atom = observedPullRequestSummaryAtom(key); - appAtomRegistry.modify(atom, (previous) => { + registry.modify(observedPullRequestSummaryAtom(key), (previous) => { const next = newestPullRequestObservation(previous, { summary, observedAt }); return next === previous ? [false, previous] : [true, next]; }); } +interface Answered { + readonly value: A; + readonly at: number; +} + +export interface LivePullRequestAnswers { + readonly detail: Answered | null; + readonly checks: Answered | null; +} + +/** Detail answers carry checks too, so whichever of the two arrived last decides them. */ +export function livePullRequestObservation( + answers: LivePullRequestAnswers, +): ObservedPullRequestSummary | null { + const { detail, checks } = answers; + if (detail === null) return null; + const live = resolvePullRequestDetailChecks( + detail.value, + checks?.value ?? null, + checks !== null && checks.at >= detail.at, + ); + if (live === null) return null; + // Checks are reread far more often than detail. Ordered by the detail's read time, a newer + // checks read would tie with the summary that detail already placed and be dropped; ordered by + // its own, a list read taken before it still loses. + const checksReadAt = live === detail.value ? undefined : checks?.value?.observedAt; + const observedAt = + checksReadAt === undefined ? live.observedAt : Math.max(live.observedAt ?? 0, checksReadAt); + return { + summary: { + ...live, + ...(observedAt === undefined ? {} : { observedAt }), + checksState: pullRequestChecksState(live.checks), + }, + observedAt: Math.max(detail.at, checks?.at ?? 0), + }; +} + +const livePullRequestAnswersAtom = Atom.family((key: string) => + Atom.make({ detail: null, checks: null }).pipe( + Atom.setIdleTTL(5 * 60_000), + Atom.withLabel(`web-pull-requests:live-answers:${key}`), + ), +); + +function recordLiveAnswer( + registry: AtomRegistry.AtomRegistry, + key: string, + patch: Partial, +): void { + const atom = livePullRequestAnswersAtom(key); + const answers = { ...registry.get(atom), ...patch }; + registry.set(atom, answers); + const observation = livePullRequestObservation(answers); + if (observation !== null) { + observePullRequestSummary(registry, key, observation.summary, observation.observedAt); + } +} + +function snapshotStorage(): Storage | undefined { + return typeof window === "undefined" ? undefined : window.localStorage; +} + +// Answers are stamped on arrival: older servers leave no other ordering for same-dated reads. +export const pullRequestEnvironment = createPullRequestEnvironmentAtoms(connectionAtomRuntime, { + list: ({ environmentId }, answer, registry) => + Effect.map(Clock.currentTimeMillis, (observedAt) => { + for (const entry of answer.entries) { + observePullRequestSummary( + registry, + pullRequestSummaryKey(environmentId, entry), + pullRequestListEntryToSummary(entry), + observedAt, + ); + } + }), + detail: ({ environmentId, input }, detail, registry) => + Effect.map(Clock.currentTimeMillis, (at) => { + // Only fresh reads seed the cold-start snapshot; a stale one could replace a newer record. + if (input.allowStale === false) { + writePullRequestDetailSnapshot(snapshotStorage(), environmentId, input, detail); + } + recordLiveAnswer(registry, pullRequestSummaryKey(environmentId, input), { + detail: { value: detail, at }, + }); + }), + checks: ({ environmentId, input }, checks, registry) => + Effect.map(Clock.currentTimeMillis, (at) => + recordLiveAnswer(registry, pullRequestSummaryKey(environmentId, input), { + checks: { value: checks, at }, + }), + ), +}); + +export const linkedPullRequestDetailAtom = createLinkedPullRequestSummaryAtomFamily( + connectionAtomRuntime, + pullRequestEnvironment.refreshes, + ({ environmentId, input }, summary, registry) => + Effect.map(Clock.currentTimeMillis, (observedAt) => + observePullRequestSummary( + registry, + pullRequestSummaryKey(environmentId, input), + summary, + observedAt, + ), + ), +); + export function threadPullRequestLinkToSummary( link: ThreadPullRequestLink, projectId: ProjectId, @@ -196,18 +304,16 @@ const savedPullRequestSummaryAtom = Atom.family((key: string) => ), ); +/** The newest of every list, detail, checks and linked-summary answer and the thread's saved link. */ export function useSharedPullRequestSummary( environmentId: EnvironmentId | null, reference: PullRequestRef | null, - current: PullRequestSummary | null, - observedAt: number | null = null, ): PullRequestSummary | null { const key = environmentId === null || reference === null ? "none" : pullRequestSummaryKey(environmentId, reference); - const atom = observedPullRequestSummaryAtom(key); - const observed = useAtomValue(atom); + const observed = useAtomValue(observedPullRequestSummaryAtom(key)); const saved = useAtomValue( savedPullRequestSummaryAtom( environmentId === null || reference === null @@ -215,16 +321,7 @@ export function useSharedPullRequestSummary( : savedPullRequestKey(environmentId, reference), ), ); - useLayoutEffect(() => { - if (key === "none" || current === null || observedAt === null) return; - observePullRequestSummary(key, current, observedAt); - }, [current, key, observedAt]); - const summary = - newestPullRequestObservation( - observed, - current === null || observedAt === null ? null : { summary: current, observedAt }, - )?.summary ?? current; - return newestPullRequestSummary(summary, saved); + return newestPullRequestSummary(observed?.summary ?? null, saved); } export const pullRequestStackAtom = createPullRequestStackAtomFamily( connectionAtomRuntime, @@ -240,7 +337,6 @@ interface MergedEnvironmentQueryView { readonly values: ReadonlyArray; readonly error: string | null; readonly isPending: boolean; - readonly observations: ReadonlyArray; } // Use one derived atom for variable fan-out; unreachable environments must not hide other rows. @@ -254,7 +350,6 @@ function createMergedEnvironmentQuery( Atom.make((get): MergedEnvironmentQueryView => { const targets = JSON.parse(key) as ReadonlyArray>; const values: Array = []; - const observations: Array = []; let error: string | null = null; let isPending = false; for (const target of targets) { @@ -265,16 +360,12 @@ function createMergedEnvironmentQuery( } const value = Option.getOrNull(AsyncResult.value(result)); if (value !== null) values.push([target.environmentId, value]); - if (result._tag === "Success") { - observations.push([target.environmentId, result.value, result.timestamp]); - } } - return { values, error, isPending, observations }; + return { values, error, isPending }; }).pipe(Atom.withLabel(`${label}:${key}`)), ); const empty = Atom.make>({ values: [], - observations: [], error: null, isPending: false, }).pipe(Atom.withLabel(`${label}:empty`)); @@ -337,22 +428,6 @@ export function usePullRequestList( targets: ReadonlyArray>, ): MergedPullRequestListView { const query = usePullRequestListsQuery(targets); - useLayoutEffect(() => { - for (const [environmentId, answer, observedAt] of query.observations) { - for (const entry of answer.entries) { - observePullRequestSummary( - pullRequestSummaryKey(environmentId, { - projectId: entry.projectId, - host: entry.host, - repository: entry.repository, - number: entry.number, - }), - pullRequestListEntryToSummary(entry), - observedAt, - ); - } - } - }, [query.observations]); const data = mergePullRequestLists(query.values); return { data, error: query.error, isPending: query.isPending, refresh: query.refresh }; } diff --git a/packages/client-runtime/src/state/pullRequests.ts b/packages/client-runtime/src/state/pullRequests.ts index 14868d7f37493..382f0ca47f653 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -13,6 +13,7 @@ import { type EnvironmentId, type PullRequestActor, type PullRequestActionInput, + type PullRequestListInput, type PullRequestDetail, type PullRequestDiffInput, type PullRequestRef, @@ -36,6 +37,7 @@ import { createEnvironmentRpcQueryAtomFamily, createEnvironmentRpcSubscriptionAtomFamily, createEnvironmentQueryAtomFamily, + type QueryAnswerHandler, } from "./runtime.ts"; import { createPullRequestRouter } from "./pullRequestRouting.ts"; import * as PullRequestDiffLoader from "./pullRequestDiffHttp.ts"; @@ -123,6 +125,7 @@ function createPullRequestRefreshAtomFamily( export function createLinkedPullRequestSummaryAtomFamily( runtime: Atom.AtomRuntime, refreshes = createPullRequestRefreshAtomFamily(runtime), + onAnswer?: QueryAnswerHandler, ) { const routedRequest = createPullRequestRouter(); return createEnvironmentRpcQueryAtomFamily(runtime, { @@ -134,9 +137,17 @@ export function createLinkedPullRequestSummaryAtomFamily( idleTtlMs: LINKED_PULL_REQUEST_IDLE_TTL_MS, refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), isFinal: (summary) => summary.state === "merged", + ...(onAnswer === undefined ? {} : { onAnswer }), }); } +/** Lets a client record what each answer says where it lands, rather than watching the data. */ +export interface PullRequestAnswerHandlers { + readonly list?: QueryAnswerHandler; + readonly detail?: QueryAnswerHandler; + readonly checks?: QueryAnswerHandler; +} + export function createPullRequestStackAtomFamily( runtime: Atom.AtomRuntime, refreshes = createPullRequestRefreshAtomFamily(runtime), @@ -172,6 +183,7 @@ export function createPullRequestEnvironmentAtoms( EnvironmentRegistry | PullRequestDiffLoader.PullRequestDiffLoader | R, E >, + onAnswer: PullRequestAnswerHandlers = {}, ) { const refreshes = createPullRequestRefreshAtomFamily(runtime); const commandScheduler = createAtomCommandScheduler(); @@ -198,6 +210,7 @@ export function createPullRequestEnvironmentAtoms( execute: (input) => routedRequest(WS_METHODS.pullRequestsDetail, input), staleTimeMs: 60_000, refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), + ...(onAnswer.detail === undefined ? {} : { onAnswer: onAnswer.detail }), }), ); const preview = writableQueryFamily( @@ -329,6 +342,7 @@ export function createPullRequestEnvironmentAtoms( staleTimeMs: 30_000, refreshTrigger: ({ environmentId, input }) => input.cursors === undefined ? refreshes({ environmentId, input: {} }) : undefined, + ...(onAnswer.list === undefined ? {} : { onAnswer: onAnswer.list }), }), /** * Its own query because the listing is quicker without them — measured over twelve @@ -358,6 +372,7 @@ export function createPullRequestEnvironmentAtoms( execute: (input) => routedRequest(WS_METHODS.pullRequestsChecks, input), staleTimeMs: 45_000, refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), + ...(onAnswer.checks === undefined ? {} : { onAnswer: onAnswer.checks }), }), ), activity, diff --git a/packages/client-runtime/src/state/runtime.test.ts b/packages/client-runtime/src/state/runtime.test.ts index b563bd8a04af8..5b348d012027c 100644 --- a/packages/client-runtime/src/state/runtime.test.ts +++ b/packages/client-runtime/src/state/runtime.test.ts @@ -44,6 +44,7 @@ import { createRuntimeCommand, scheduleAtomCommandEffect, executeAtomQuery, + type QueryAnswerHandler, } from "./runtime.ts"; const QUERY_ENVIRONMENT = new PrimaryConnectionTarget({ @@ -92,8 +93,10 @@ const makeEnvironmentQueryHarness = Effect.fn("TestEnvironmentQuery.makeHarness" initialState, rpcSession, isFinal, + onAnswer, }: { readonly refreshTrigger?: Atom.Atom; + readonly onAnswer?: QueryAnswerHandler; readonly isFinal?: (value: A) => boolean; readonly idleTtlMs?: number; readonly persistence?: QueryPersistence; @@ -142,6 +145,7 @@ const makeEnvironmentQueryHarness = Effect.fn("TestEnvironmentQuery.makeHarness" ...(refreshTrigger === undefined ? {} : { refreshTrigger: () => refreshTrigger }), ...(idleTtlMs === undefined ? {} : { idleTtlMs }), ...(isFinal === undefined ? {} : { isFinal }), + ...(onAnswer === undefined ? {} : { onAnswer }), }); return { @@ -1152,6 +1156,55 @@ describe("persisted environment queries", () => { ), ); + it.effect("hands each server answer to onAnswer before readers see it", () => + Effect.scoped( + Effect.gen(function* () { + const answers: Array = []; + let served = 0; + const recorded = Atom.make(null).pipe(Atom.keepAlive); + const harness = yield* makeEnvironmentQueryHarness( + Effect.sync(() => `answer-${++served}`), + { + onAnswer: (_target, value, registry) => + Effect.sync(() => { + answers.push([value, registry.get(recorded)]); + registry.set(recorded, value); + }), + }, + ); + const registry = yield* mountEnvironmentQuery(harness.atom); + expect(yield* readEnvironmentQuery(registry, harness.atom)).toBe("answer-1"); + expect(registry.get(recorded)).toBe("answer-1"); + registry.refresh(harness.atom); + expect(yield* readEnvironmentQuery(registry, harness.atom)).toBe("answer-2"); + expect(answers).toEqual([ + ["answer-1", null], + ["answer-2", "answer-1"], + ]); + }), + ), + ); + + it.effect("does not hand a value restored from persistence to onAnswer", () => + Effect.scoped( + Effect.gen(function* () { + const answers: Array = []; + const harness = yield* makeEnvironmentQueryHarness(Effect.succeed("server"), { + persistence: { load: () => Effect.succeedSome("cached"), save: () => Effect.void }, + initialState: queryConnectionState({ phase: "offline", network: "offline" }), + onAnswer: (_target, value) => Effect.sync(() => answers.push(value)), + }); + const registry = yield* mountEnvironmentQuery(harness.atom); + expect(yield* readEnvironmentQuery(registry, harness.atom)).toBe("cached"); + expect(answers).toEqual([]); + yield* SubscriptionRef.set(harness.supervisorState, queryConnectionState()); + yield* Effect.yieldNow; + expect(yield* readEnvironmentQuery(registry, harness.atom)).toBe("server"); + expect(answers).toEqual(["server"]); + }), + ), + ); + it.effect("keeps cached content available when revalidation fails", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/client-runtime/src/state/runtime.ts b/packages/client-runtime/src/state/runtime.ts index 5a408d61e9219..79b1ec4ac5e81 100644 --- a/packages/client-runtime/src/state/runtime.ts +++ b/packages/client-runtime/src/state/runtime.ts @@ -101,8 +101,20 @@ interface EnvironmentQueryAtomOptions extends Omit< readonly input: Input; }) => Atom.Atom | undefined; readonly isFinal?: (value: A) => boolean; + /** + * Runs where each server answer lands, before readers see it, so derived records (summaries, + * snapshots, watches) come from the producer instead of from components watching the data. + * Cached and emitted provisional values are not answers. + */ + readonly onAnswer?: QueryAnswerHandler; } +export type QueryAnswerHandler = ( + target: { readonly environmentId: EnvironmentIdType; readonly input: Input }, + value: A, + registry: AtomRegistry.AtomRegistry, +) => Effect.Effect; + interface EnvironmentSubscriptionAtomOptions { readonly sensitiveInput?: boolean; readonly label: string; @@ -652,7 +664,15 @@ export function createEnvironmentQueryAtomFamily( ); } }; - if (!options.persistence) return execute().pipe(Effect.map(share)); + const onAnswer = options.onAnswer; + const answer = () => + onAnswer === undefined + ? execute().pipe(Effect.map(share)) + : execute().pipe( + Effect.map(share), + Effect.tap((value) => onAnswer(target, value, get.registry)), + ); + if (!options.persistence) return answer(); const persistence = options.persistence; const cacheKey = JSON.stringify([options.label, target.input]); const previous = Option.flatMap( @@ -670,8 +690,7 @@ export function createEnvironmentQueryAtomFamily( return cached.value; } if (Option.isNone(previous) && Option.isSome(cached)) yield* emit(cached.value); - return yield* execute().pipe( - Effect.map(share), + return yield* answer().pipe( Effect.tap((value) => persistence.save(target.environmentId, cacheKey, value)), ); }); @@ -776,6 +795,7 @@ export function createEnvironmentRpcQueryAtomFamily< readonly input: EnvironmentRpcInput; }) => Atom.Atom | undefined; readonly isFinal?: (value: EnvironmentRpcSuccess) => boolean; + readonly onAnswer?: QueryAnswerHandler, EnvironmentRpcSuccess>; }, ) { return createEnvironmentQueryAtomFamily(runtime, { @@ -788,6 +808,7 @@ export function createEnvironmentRpcQueryAtomFamily< : { refreshIntervalMs: options.refreshIntervalMs }), ...(options.refreshTrigger === undefined ? {} : { refreshTrigger: options.refreshTrigger }), ...(options.isFinal === undefined ? {} : { isFinal: options.isFinal }), + ...(options.onAnswer === undefined ? {} : { onAnswer: options.onAnswer }), execute: ( input: EnvironmentRpcInput, emit: (value: EnvironmentRpcSuccess) => Effect.Effect, From f3aea790e5ef6347207fbbb108fc9a1c689b5f32 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Sun, 25 Oct 2026 15:15:15 -0700 Subject: [PATCH 2/4] chore: lower the effect budget for the removed effects Co-Authored-By: Claude Opus 5.5 --- scripts/effect-budget.json | 2 -- 1 file changed, 2 deletions(-) diff --git a/scripts/effect-budget.json b/scripts/effect-budget.json index 008ba170fd03f..96b75a99a67fb 100644 --- a/scripts/effect-budget.json +++ b/scripts/effect-budget.json @@ -350,7 +350,6 @@ "apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx": 2, "apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts": 1, "apps/web/src/components/pullRequest/useInlineActionConfirm.ts": 1, - "apps/web/src/components/pullRequest/usePullRequestDetail.ts": 1, "apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts": 2, "apps/web/src/components/pullRequest/usePullRequestListView.ts": 1, "apps/web/src/components/pullRequest/useThreadLinkedPullRequest.ts": 1, @@ -430,7 +429,6 @@ "apps/web/src/routes/_chat.tsx": 1, "apps/web/src/shortcutModifierState.ts": 1, "apps/web/src/state/olderSettledShelf.ts": 2, - "apps/web/src/state/pullRequests.ts": 2, "apps/web/src/state/queries.ts": 2, "apps/web/src/state/threads.ts": 1 } From c0d81a0334f0d48a923603f329d6f54f59be298d Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Sun, 25 Oct 2026 15:22:05 -0700 Subject: [PATCH 3/4] chore: leave the effect budget to a separate sweep Co-Authored-By: Claude Opus 5.5 --- scripts/effect-budget.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/effect-budget.json b/scripts/effect-budget.json index 96b75a99a67fb..008ba170fd03f 100644 --- a/scripts/effect-budget.json +++ b/scripts/effect-budget.json @@ -350,6 +350,7 @@ "apps/web/src/components/pullRequest/guide/PullRequestGuideRunCoordinator.tsx": 2, "apps/web/src/components/pullRequest/guide/useReviewGuideWriter.ts": 1, "apps/web/src/components/pullRequest/useInlineActionConfirm.ts": 1, + "apps/web/src/components/pullRequest/usePullRequestDetail.ts": 1, "apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts": 2, "apps/web/src/components/pullRequest/usePullRequestListView.ts": 1, "apps/web/src/components/pullRequest/useThreadLinkedPullRequest.ts": 1, @@ -429,6 +430,7 @@ "apps/web/src/routes/_chat.tsx": 1, "apps/web/src/shortcutModifierState.ts": 1, "apps/web/src/state/olderSettledShelf.ts": 2, + "apps/web/src/state/pullRequests.ts": 2, "apps/web/src/state/queries.ts": 2, "apps/web/src/state/threads.ts": 1 } From ac88f7c2ffc4d8d392574b9c7adfc7625012f1b6 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Sun, 25 Oct 2026 16:47:29 -0700 Subject: [PATCH 4/4] fix(web): checks refreshes no longer bring back old mergeability A checks read used to restamp the whole detail summary with its newer read time, so the detail's older metadata could beat a newer list read. Metadata and checks now keep separate freshness: a checks read updates only the checks state, and metadata takes only the newest metadata observation. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/state/pullRequests.test.ts | 109 ++++++++------- apps/web/src/state/pullRequests.ts | 170 +++++++++++++----------- 2 files changed, 157 insertions(+), 122 deletions(-) diff --git a/apps/web/src/state/pullRequests.test.ts b/apps/web/src/state/pullRequests.test.ts index 140b1a78315eb..d524501740cee 100644 --- a/apps/web/src/state/pullRequests.test.ts +++ b/apps/web/src/state/pullRequests.test.ts @@ -1,15 +1,12 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; -import { - ProjectId, - type PullRequestCheck, - type PullRequestDetail, - type PullRequestSummary, -} from "@t3tools/contracts"; +import { ProjectId, type PullRequestSummary } from "@t3tools/contracts"; import { describe, expect, it } from "vite-plus/test"; import { - livePullRequestObservation, newestPullRequestObservation, + observedPullRequestSummary, + observePullRequestChecks, + observePullRequestMetadata, newestPullRequestSummary, threadPullRequestLinkToSummary, } from "./pullRequests"; @@ -121,50 +118,72 @@ describe("pull request summary cache", () => { }); }); -describe("live pull request answers", () => { - const failing: PullRequestCheck = { - name: "build", - status: "failure", - url: null, - description: null, - }; - const detail = { ...summary(), checks: [] } as unknown as PullRequestDetail; +describe("pull request observations", () => { + const fromSummary = (value: PullRequestSummary, observedAt: number) => + observePullRequestMetadata({ metadata: null, checks: null }, observed(value, observedAt)); + const checksRead = ( + checksState: "passing" | "failing" | "pending" | null, + readAt: number | undefined, + observedAt: number, + ) => ({ checksState, readAt, observedAt }); - it("lets whichever of detail and checks answered last decide the checks", () => { - const checks = { value: { state: "open" as const, checks: [failing] }, at: 200 }; - expect(livePullRequestObservation({ detail: { value: detail, at: 100 }, checks })).toEqual({ - summary: { ...detail, checks: [failing], checksState: "failing" }, - observedAt: 200, + it("keeps a newer list's mergeability when a checks read lands after it", () => { + const detail = summary({ + mergeability: "conflicting", + checksState: "pending", + observedAt: 100, + }); + let state = fromSummary(detail, 1); + state = observePullRequestMetadata( + state, + observed(summary({ mergeability: "mergeable", checksState: "pending", observedAt: 200 }), 2), + ); + state = observePullRequestChecks(state, checksRead("passing", 300, 3)); + expect(observedPullRequestSummary(state)).toMatchObject({ + mergeability: "mergeable", + checksState: "passing", }); - expect( - livePullRequestObservation({ detail: { value: detail, at: 300 }, checks }), - ).toMatchObject({ summary: { checks: [], checksState: null }, observedAt: 300 }); }); - it("lets a newer checks read replace the detail's checks while stale list reads stay out", () => { - const pending: PullRequestCheck = { ...failing, status: "pending" }; - const passing: PullRequestCheck = { ...failing, status: "success" }; - const read = { ...detail, observedAt: 100, checks: [pending] } as PullRequestDetail; - const fromDetail = livePullRequestObservation({ - detail: { value: read, at: 1_000 }, - checks: null, - }); - const fromChecks = livePullRequestObservation({ - detail: { value: read, at: 1_000 }, - checks: { value: { observedAt: 300, state: "open", checks: [passing] }, at: 2_000 }, + it("keeps a newer checks read when an older list read lands after it", () => { + let state = fromSummary( + summary({ mergeability: "conflicting", checksState: "pending", observedAt: 100 }), + 1, + ); + state = observePullRequestChecks(state, checksRead("passing", 300, 2)); + state = observePullRequestMetadata( + state, + observed(summary({ mergeability: "mergeable", checksState: "pending", observedAt: 250 }), 3), + ); + expect(observedPullRequestSummary(state)).toMatchObject({ + mergeability: "mergeable", + checksState: "passing", }); - const accepted = newestPullRequestObservation(fromDetail, fromChecks); - expect(accepted?.summary.checksState).toBe("passing"); - const staleList = observed(summary({ checksState: "pending", observedAt: 200 }), 3_000); - expect(newestPullRequestObservation(accepted, staleList)).toBe(accepted); }); - it("waits for a detail answer before observing checks", () => { - expect( - livePullRequestObservation({ - detail: null, - checks: { value: { state: "open", checks: [failing] }, at: 1 }, - }), - ).toBeNull(); + it("lets a newer read's checks replace an older checks read", () => { + let state = fromSummary(summary({ checksState: "pending", observedAt: 100 }), 1); + state = observePullRequestChecks(state, checksRead("passing", 300, 2)); + state = observePullRequestMetadata( + state, + observed(summary({ checksState: "failing", observedAt: 400 }), 3), + ); + expect(observedPullRequestSummary(state)?.checksState).toBe("failing"); + }); + + it("orders checks by arrival when the server reports no read time", () => { + let state = fromSummary(summary({ checksState: "pending" }), 1); + state = observePullRequestChecks(state, checksRead("passing", undefined, 2)); + expect(observedPullRequestSummary(state)?.checksState).toBe("passing"); + state = observePullRequestChecks(state, checksRead("failing", undefined, 1)); + expect(observedPullRequestSummary(state)?.checksState).toBe("passing"); + }); + + it("shows nothing until some read has described the pull request", () => { + const state = observePullRequestChecks( + { metadata: null, checks: null }, + checksRead("failing", 1, 1), + ); + expect(observedPullRequestSummary(state)).toBeNull(); }); }); diff --git a/apps/web/src/state/pullRequests.ts b/apps/web/src/state/pullRequests.ts index d151825451024..e8ec9a7a40728 100644 --- a/apps/web/src/state/pullRequests.ts +++ b/apps/web/src/state/pullRequests.ts @@ -8,8 +8,7 @@ import { import type { EnvironmentId, ProjectId, - PullRequestChecks, - PullRequestDetail, + PullRequestChecksState, PullRequestListInput, PullRequestFilterOptionsInput, PullRequestListStatsInput, @@ -22,7 +21,6 @@ import { threadPullRequestKeyOf, visibleThreadPullRequests, } from "@t3tools/client-runtime/thread-pull-requests"; -import { resolvePullRequestDetailChecks } from "@t3tools/client-runtime/pull-request-checks"; import * as Clock from "effect/Clock"; import * as Effect from "effect/Effect"; import * as Option from "effect/Option"; @@ -46,13 +44,6 @@ export interface ObservedPullRequestSummary { readonly observedAt: number; } -const observedPullRequestSummaryAtom = Atom.family((key: string) => - Atom.make(null).pipe( - Atom.setIdleTTL(5 * 60_000), - Atom.withLabel(`web-pull-requests:observed-summary:${key}`), - ), -); - function compareSummaries(current: PullRequestSummary, incoming: PullRequestSummary): number { const merged = Number(incoming.state === "merged") - Number(current.state === "merged"); if (merged !== 0) return merged; @@ -136,75 +127,92 @@ export function newestPullRequestObservation( }; } -function observePullRequestSummary( - registry: AtomRegistry.AtomRegistry, - key: string, - summary: PullRequestSummary, - observedAt: number, -): void { - registry.modify(observedPullRequestSummaryAtom(key), (previous) => { - const next = newestPullRequestObservation(previous, { summary, observedAt }); - return next === previous ? [false, previous] : [true, next]; - }); -} - -interface Answered { - readonly value: A; - readonly at: number; +export interface ObservedPullRequestChecks { + readonly checksState: PullRequestChecksState | null; + /** Server read time, when the server reports one. */ + readonly readAt: number | undefined; + /** Client arrival time. */ + readonly observedAt: number; } -export interface LivePullRequestAnswers { - readonly detail: Answered | null; - readonly checks: Answered | null; +/** + * Checks are reread far more often than the rest of a pull request, so they keep their own + * freshness: a checks read must not carry old metadata forward, and a list read must not take + * back newer checks. + */ +export interface PullRequestObservations { + readonly metadata: ObservedPullRequestSummary | null; + readonly checks: ObservedPullRequestChecks | null; } -/** Detail answers carry checks too, so whichever of the two arrived last decides them. */ -export function livePullRequestObservation( - answers: LivePullRequestAnswers, -): ObservedPullRequestSummary | null { - const { detail, checks } = answers; - if (detail === null) return null; - const live = resolvePullRequestDetailChecks( - detail.value, - checks?.value ?? null, - checks !== null && checks.at >= detail.at, - ); - if (live === null) return null; - // Checks are reread far more often than detail. Ordered by the detail's read time, a newer - // checks read would tie with the summary that detail already placed and be dropped; ordered by - // its own, a list read taken before it still loses. - const checksReadAt = live === detail.value ? undefined : checks?.value?.observedAt; - const observedAt = - checksReadAt === undefined ? live.observedAt : Math.max(live.observedAt ?? 0, checksReadAt); - return { - summary: { - ...live, - ...(observedAt === undefined ? {} : { observedAt }), - checksState: pullRequestChecksState(live.checks), - }, - observedAt: Math.max(detail.at, checks?.at ?? 0), - }; -} +const noObservations: PullRequestObservations = { metadata: null, checks: null }; -const livePullRequestAnswersAtom = Atom.family((key: string) => - Atom.make({ detail: null, checks: null }).pipe( +const pullRequestObservationsAtom = Atom.family((key: string) => + Atom.make(noObservations).pipe( Atom.setIdleTTL(5 * 60_000), - Atom.withLabel(`web-pull-requests:live-answers:${key}`), + Atom.withLabel(`web-pull-requests:observations:${key}`), ), ); -function recordLiveAnswer( +export function observePullRequestChecks( + current: PullRequestObservations, + incoming: ObservedPullRequestChecks, +): PullRequestObservations { + const previous = current.checks; + const newer = + previous === null || + (previous.readAt !== undefined && incoming.readAt !== undefined + ? incoming.readAt >= previous.readAt + : incoming.observedAt >= previous.observedAt); + return newer ? { ...current, checks: incoming } : current; +} + +export function observePullRequestMetadata( + current: PullRequestObservations, + incoming: ObservedPullRequestSummary, +): PullRequestObservations { + const metadata = newestPullRequestObservation(current.metadata, incoming); + const next = metadata === current.metadata ? current : { ...current, metadata }; + const { checksState } = incoming.summary; + return checksState === undefined + ? next + : observePullRequestChecks(next, { + checksState, + readAt: incoming.summary.observedAt, + observedAt: incoming.observedAt, + }); +} + +export function observedPullRequestSummary( + observations: PullRequestObservations, +): PullRequestSummary | null { + const { metadata, checks } = observations; + if (metadata === null) return null; + return checks === null + ? metadata.summary + : { ...metadata.summary, checksState: checks.checksState }; +} + +function recordObservation( registry: AtomRegistry.AtomRegistry, key: string, - patch: Partial, + observe: (current: PullRequestObservations) => PullRequestObservations, ): void { - const atom = livePullRequestAnswersAtom(key); - const answers = { ...registry.get(atom), ...patch }; - registry.set(atom, answers); - const observation = livePullRequestObservation(answers); - if (observation !== null) { - observePullRequestSummary(registry, key, observation.summary, observation.observedAt); - } + registry.modify(pullRequestObservationsAtom(key), (previous) => { + const next = observe(previous); + return next === previous ? [false, previous] : [true, next]; + }); +} + +function observePullRequestSummary( + registry: AtomRegistry.AtomRegistry, + key: string, + summary: PullRequestSummary, + observedAt: number, +): void { + recordObservation(registry, key, (current) => + observePullRequestMetadata(current, { summary, observedAt }), + ); } function snapshotStorage(): Storage | undefined { @@ -230,16 +238,24 @@ export const pullRequestEnvironment = createPullRequestEnvironmentAtoms(connecti if (input.allowStale === false) { writePullRequestDetailSnapshot(snapshotStorage(), environmentId, input, detail); } - recordLiveAnswer(registry, pullRequestSummaryKey(environmentId, input), { - detail: { value: detail, at }, - }); + observePullRequestSummary( + registry, + pullRequestSummaryKey(environmentId, input), + { ...detail, checksState: pullRequestChecksState(detail.checks) }, + at, + ); }), checks: ({ environmentId, input }, checks, registry) => - Effect.map(Clock.currentTimeMillis, (at) => - recordLiveAnswer(registry, pullRequestSummaryKey(environmentId, input), { - checks: { value: checks, at }, - }), - ), + Effect.map(Clock.currentTimeMillis, (observedAt) => { + if (checks === null) return; + recordObservation(registry, pullRequestSummaryKey(environmentId, input), (current) => + observePullRequestChecks(current, { + checksState: pullRequestChecksState(checks.checks), + readAt: checks.observedAt, + observedAt, + }), + ); + }), }); export const linkedPullRequestDetailAtom = createLinkedPullRequestSummaryAtomFamily( @@ -313,7 +329,7 @@ export function useSharedPullRequestSummary( environmentId === null || reference === null ? "none" : pullRequestSummaryKey(environmentId, reference); - const observed = useAtomValue(observedPullRequestSummaryAtom(key)); + const observations = useAtomValue(pullRequestObservationsAtom(key)); const saved = useAtomValue( savedPullRequestSummaryAtom( environmentId === null || reference === null @@ -321,7 +337,7 @@ export function useSharedPullRequestSummary( : savedPullRequestKey(environmentId, reference), ), ); - return newestPullRequestSummary(observed?.summary ?? null, saved); + return newestPullRequestSummary(observedPullRequestSummary(observations), saved); } export const pullRequestStackAtom = createPullRequestStackAtomFamily( connectionAtomRuntime,