Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ import {
allowsSinglePullRequestMerge,
canDeletePullRequestBranch,
isPullRequestConflicting,
pullRequestDetailPatchAfterAction,
resolveBaseFreshness,
resolvePullRequestMergeMethod,
type PullRequestFinding,
Expand Down Expand Up @@ -347,6 +348,7 @@ export function PullRequestDetailPanel({
const [threadPickerOpen, setThreadPickerOpen] = useState(false);
const [openReviewRequest, setOpenReviewRequest] = useState(0);
const {
recordActionOutcome,
canWriteSourceControl,
detailQuery,
activityQuery,
Expand Down Expand Up @@ -687,6 +689,8 @@ export function PullRequestDetailPanel({
onActed?.(action, "failed");
return false;
}
const outcome = pullRequestDetailPatchAfterAction(action);
if (outcome !== null) recordActionOutcome(outcome);
if (action === "revert") {
toastManager.add({ type: "success", title: `Revert ${terminology.singular} opened` });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ import {
pullRequestPanelContext,
threadPullRequestPanelTarget,
latestPullRequestReviewOutcomes,
applyPullRequestDetailOverride,
pullRequestDetailPatchAfterAction,
recordPullRequestDetailOverride,
settlePullRequestDetailOverride,
newestPullRequestCommitAt,
mergePullRequestThreadComments,
orderPullRequestComments,
Expand Down Expand Up @@ -315,6 +319,78 @@ describe("review verdicts", () => {
});
});

describe("confirmed action outcome on the detail", () => {
const scope = "env:project:host:repo#1";
const detail = { state: "open" as const, isDraft: true, autoMergeEnabled: false };
const at = 1_000_000;
const record = (patch: NonNullable<ReturnType<typeof pullRequestDetailPatchAfterAction>>) =>
recordPullRequestDetailOverride(null, scope, patch, at);

it("maps each action to the state it confirms, and leaves merge and the rest alone", () => {
expect(
(
["ready", "draft", "close", "reopen", "enable-auto-merge", "disable-auto-merge"] as const
).map(pullRequestDetailPatchAfterAction),
).toEqual([
{ isDraft: false },
{ isDraft: true },
{ state: "closed" },
{ state: "open" },
{ autoMergeEnabled: true },
{ autoMergeEnabled: false },
]);
expect(pullRequestDetailPatchAfterAction("merge")).toBeNull();
});

it("shows the outcome over the fetched detail", () => {
expect(applyPullRequestDetailOverride(detail, record({ isDraft: false }))).toMatchObject({
state: "open",
isDraft: false,
});
expect(applyPullRequestDetailOverride(detail, record({ state: "closed" })).state).toBe(
"closed",
);
expect(applyPullRequestDetailOverride(detail, null)).toBe(detail);
});

it("accumulates outcomes within a pull request and starts over for another", () => {
const first = record({ isDraft: false });
expect(recordPullRequestDetailOverride(first, scope, { state: "closed" }, at + 1)).toEqual({
scope,
at: at + 1,
isDraft: false,
state: "closed",
});
expect(recordPullRequestDetailOverride(first, "other", { state: "closed" }, at + 1)).toEqual({
scope: "other",
at: at + 1,
state: "closed",
});
});

it("retires a field once the fetched detail agrees", () => {
const override = recordPullRequestDetailOverride(
record({ isDraft: false }),
scope,
{ autoMergeEnabled: true },
at,
);
expect(settlePullRequestDetailOverride(override, detail, scope)).toBe(override);
expect(settlePullRequestDetailOverride(override, { ...detail, isDraft: false }, scope)).toEqual(
{ scope, at, autoMergeEnabled: true },
);
expect(
settlePullRequestDetailOverride(
override,
{ ...detail, isDraft: false, autoMergeEnabled: true },
scope,
),
).toBeNull();
expect(settlePullRequestDetailOverride(override, detail, "other")).toBeNull();
expect(settlePullRequestDetailOverride(override, null, scope)).toBe(override);
});
});

describe("pull request timeline", () => {
it("orders host timestamps by instant across offsets and fractional seconds", () => {
const events = buildPullRequestTimeline({
Expand Down
89 changes: 89 additions & 0 deletions apps/web/src/components/pullRequest/pullRequestDetail.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1403,3 +1403,92 @@ export function resolveDisplayedPullRequestDetail(input: {
? input.cached
: null;
}

export interface PullRequestDetailPatch {
readonly state?: PullRequestState;
readonly isDraft?: boolean;
readonly autoMergeEnabled?: boolean;
}

export interface PullRequestDetailOverride extends PullRequestDetailPatch {
readonly scope: string;
readonly at: number;
}

export function pullRequestDetailPatchAfterAction(
action: PullRequestAction,
): PullRequestDetailPatch | null {
switch (action) {
case "ready":
return { isDraft: false };
case "draft":
return { isDraft: true };
case "close":
return { state: "closed" };
case "reopen":
return { state: "open" };
case "enable-auto-merge":
return { autoMergeEnabled: true };
case "disable-auto-merge":
return { autoMergeEnabled: false };
default:
return null;
}
}

export function recordPullRequestDetailOverride(
previous: PullRequestDetailOverride | null,
scope: string,
patch: PullRequestDetailPatch,
now: number,
): PullRequestDetailOverride {
return { ...(previous?.scope === scope ? previous : {}), ...patch, scope, at: now };
Comment thread
Brechard marked this conversation as resolved.
Resolved
}

type OverridableDetail = Pick<PullRequestDetail, "state" | "isDraft" | "autoMergeEnabled">;

export function applyPullRequestDetailOverride<Detail extends OverridableDetail>(
detail: Detail,
override: PullRequestDetailOverride | null,
): Detail {
if (override === null) return detail;
const { state, isDraft, autoMergeEnabled } = override;
return {
...detail,
...(state === undefined ? {} : { state }),
...(isDraft === undefined ? {} : { isDraft }),
...(autoMergeEnabled === undefined ? {} : { autoMergeEnabled }),
};
}

/** Drop what the fetched detail now agrees with; the rest holds until it expires. */
export function settlePullRequestDetailOverride(
override: PullRequestDetailOverride | null,
detail: OverridableDetail | null,
scope: string,
): PullRequestDetailOverride | null {
if (override === null) return null;
if (override.scope !== scope) return null;
if (detail === null) return override;
const { state, isDraft, autoMergeEnabled } = override;
const open = {
state: state !== undefined && detail.state !== state,
isDraft: isDraft !== undefined && detail.isDraft !== isDraft,
autoMergeEnabled:
autoMergeEnabled !== undefined && detail.autoMergeEnabled !== autoMergeEnabled,
};
if (!Object.values(open).some(Boolean)) return null;
if (
open.state === (state !== undefined) &&
open.isDraft === (isDraft !== undefined) &&
open.autoMergeEnabled === (autoMergeEnabled !== undefined)
)
return override;
return {
scope: override.scope,
at: override.at,
...(open.state ? { state } : {}),
...(open.isDraft ? { isDraft } : {}),
...(open.autoMergeEnabled ? { autoMergeEnabled } : {}),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ export function reusePullRequestEntries<Entry extends PullRequestListEntry>(
: out;
}

const PULL_REQUEST_OVERRIDE_TRUST_MS = 60_000;
export const PULL_REQUEST_OVERRIDE_TRUST_MS = 60_000;

/** Retire overrides only when returned rows agree; partial-feed absence proves nothing. */
export function settlePullRequestOverrides<Entry extends PullRequestListEntry>(
Expand Down
23 changes: 22 additions & 1 deletion apps/web/src/components/pullRequest/usePullRequestDetail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,15 @@ import {
useSharedPullRequestSummary,
} from "~/state/pullRequests";
import {
applyPullRequestDetailOverride,
readPullRequestDetailSnapshot,
recordPullRequestDetailOverride,
resolveDisplayedPullRequestDetail,
settlePullRequestDetailOverride,
type PullRequestDetailOverride,
type PullRequestDetailPatch,
} from "./pullRequestDetail.logic";
import { PULL_REQUEST_OVERRIDE_TRUST_MS } from "./pullRequestList.logic";

export function pullRequestRefKey(reference: PullRequestRef) {
return `${reference.projectId}:${reference.host ?? ""}:${reference.repository}#${reference.number}`;
Expand Down Expand Up @@ -88,6 +94,7 @@ export function usePullRequestDetail(
} else if (detailQuery.data !== null && detailQuery.data !== cachedDetail) {
setCachedDetail(detailQuery.data);
}
const [override, setOverride] = useState<PullRequestDetailOverride | null>(null);
const observedSummary = useSharedPullRequestSummary(environmentId, reference);
const activity = activityQuery.data;
const resolvedCoreDetail = resolveDisplayedPullRequestDetail({
Expand Down Expand Up @@ -128,7 +135,7 @@ export function usePullRequestDetail(
: sharedSummary.mergedAt,
isDraft: sharedSummary.isDraft ?? resolvedCoreDetail.isDraft,
};
const detail =
const fetchedDetail =
coreDetail === null
? null
: {
Expand Down Expand Up @@ -169,7 +176,21 @@ export function usePullRequestDetail(
commits: activity?.commits ?? [],
reactions: activity?.reactions ?? [],
};
const settledOverride = settlePullRequestDetailOverride(override, fetchedDetail, scopeKey);
if (settledOverride !== override) setOverride(settledOverride);
const detail =
fetchedDetail === null ? null : applyPullRequestDetailOverride(fetchedDetail, settledOverride);
const recordActionOutcome = (patch: PullRequestDetailPatch) => {
const at = Date.now();
setOverride((previous) => recordPullRequestDetailOverride(previous, scopeKey, patch, at));
// A fetch that never agrees must not pin the confirmed outcome forever.
window.setTimeout(
() => setOverride((previous) => (previous?.at === at ? null : previous)),
PULL_REQUEST_OVERRIDE_TRUST_MS,
);
};
return {
recordActionOutcome,
canWriteSourceControl,
detailQuery,
activityQuery,
Expand Down
Loading