fix(action): don't flag stacked PRs as outdated when their base PR merges

action/dist/main.js

@@ -159163,7 +159163,10 @@ async function flagOverlappingOpenPrs(params, deps) {
     const overlapping = [...changesetPaths(otherChangeset)].filter((p2) => mergingPaths.has(p2) && otherChangeset[p2] !== mergingChangeset[p2]);
     if (overlapping.length === 0)
       continue;
-    deps.core.info(`Flagging PR #${pr.number} as stale (overlapping paths: ${overlapping.join(", ")}).`);
+    const stale = await dropStackedPaths(deps, bucket, pr.number, otherChangeset, mergingChangeset, overlapping);
+    if (stale.length === 0)
+      continue;
+    deps.core.info(`Flagging PR #${pr.number} as stale (overlapping paths: ${stale.join(", ")}).`);
     await deps.octokit.rest.repos.createCommitStatus({
       ...repo,
       sha: pr.head.sha,
@@ -159175,6 +159178,18 @@ async function flagOverlappingOpenPrs(params, deps) {
   }
   return flagged;
 }
+async function dropStackedPaths(deps, bucket, prNumber, otherChangeset, mergingChangeset, overlapping) {
+  const otherHeadSha = otherChangeset[HEAD_SHA_KEY3];
+  if (!otherHeadSha)
+    return overlapping;
+  const otherBaseline = await deps.getAncestorManifest(bucket, otherHeadSha);
+  const stacked = overlapping.filter((p2) => (otherBaseline[p2] ?? null) === mergingChangeset[p2]);
+  if (stacked.length === 0)
+    return overlapping;
+  deps.core.info(`PR #${prNumber} was compared against a baseline that already includes the merging changes for ${stacked.join(", ")} — not stale for those paths.`);
+  const stackedPaths = new Set(stacked);
+  return overlapping.filter((p2) => !stackedPaths.has(p2));
+}
 function changesetPaths(changeset) {
   return new Set(Object.keys(changeset).filter((key) => key !== HEAD_SHA_KEY3));
 }
@@ -159302,6 +159317,11 @@ async function mergeEntry(bucket, entry, deps) {
     flagOverlappingOpenPrs: (params) => flagOverlappingOpenPrs(params, {
       octokit: deps.octokit,
       getChangeset: manifestS3.getChangeset,
+      getAncestorManifest: (bucket2, startSha) => findAncestorManifest(bucket2, startSha, {
+        getManifest: manifestS3.getManifest,
+        getParentSha: (sha) => getParentSha(sha, deps),
+        core: deps.core
+      }),
       core: deps.core
     }),
     applyChangesetToBaseImages: (params) => applyChangesetToBaseImages(params, {

action/src/manifest-merge-flag-prs.ts

@@ -1,12 +1,13 @@
 import { VISUAL_REGRESSION_CONTEXT } from 'shared/constants';
 import type { Dependencies } from './dependencies';
-import type { Changeset } from './manifest-s3';
+import type { Changeset, Manifest } from './manifest-s3';
 
 const HEAD_SHA_KEY = '_headSha';
 
 export interface FlagOverlappingPrsDeps {
   octokit: Dependencies['octokit'];
   getChangeset: (bucket: string, sha: string) => Promise<Changeset | null>;
+  getAncestorManifest: (bucket: string, startSha: string) => Promise<Manifest>;
   core: Pick<Dependencies['core'], 'info'>;
 }
 
@@ -56,8 +57,18 @@ export async function flagOverlappingOpenPrs(
     );
     if (overlapping.length === 0) continue;
 
+    const stale = await dropStackedPaths(
+      deps,
+      bucket,
+      pr.number,
+      otherChangeset,
+      mergingChangeset,
+      overlapping
+    );
+    if (stale.length === 0) continue;
+
     deps.core.info(
-      `Flagging PR #${pr.number} as stale (overlapping paths: ${overlapping.join(', ')}).`
+      `Flagging PR #${pr.number} as stale (overlapping paths: ${stale.join(', ')}).`
     );
     await deps.octokit.rest.repos.createCommitStatus({
       ...repo,
@@ -72,6 +83,30 @@ export async function flagOverlappingOpenPrs(
   return flagged;
 }
 
+async function dropStackedPaths(
+  deps: FlagOverlappingPrsDeps,
+  bucket: string,
+  prNumber: number,
+  otherChangeset: Changeset,
+  mergingChangeset: Changeset,
+  overlapping: string[]
+): Promise<string[]> {
+  const otherHeadSha = otherChangeset[HEAD_SHA_KEY];
+  if (!otherHeadSha) return overlapping;
+
+  const otherBaseline = await deps.getAncestorManifest(bucket, otherHeadSha);
+  const stacked = overlapping.filter(
+    p => (otherBaseline[p] ?? null) === mergingChangeset[p]
+  );
+  if (stacked.length === 0) return overlapping;
+
+  deps.core.info(
+    `PR #${prNumber} was compared against a baseline that already includes the merging changes for ${stacked.join(', ')} — not stale for those paths.`
+  );
+  const stackedPaths = new Set(stacked);
+  return overlapping.filter(p => !stackedPaths.has(p));
+}
+
 function changesetPaths(changeset: Changeset): Set<string> {
   return new Set(Object.keys(changeset).filter(key => key !== HEAD_SHA_KEY));
 }

action/src/manifest-run.ts

@@ -197,6 +197,12 @@ async function mergeEntry(
         flagOverlappingOpenPrs(params, {
           octokit: deps.octokit,
           getChangeset: manifestS3.getChangeset,
+          getAncestorManifest: (bucket, startSha) =>
+            findAncestorManifest(bucket, startSha, {
+              getManifest: manifestS3.getManifest,
+              getParentSha: sha => getParentSha(sha, deps),
+              core: deps.core
+            }),
           core: deps.core
         }),
       applyChangesetToBaseImages: params =>

action/test/manifest-merge-flag-prs.test.ts

@@ -10,6 +10,7 @@ const paginateMock = mock<any>();
 const listPullsMock = mock<any>();
 const createCommitStatusMock = mock<any>();
 const getChangesetMock = mock<any>();
+const getAncestorManifestMock = mock<any>();
 const infoMock = mock<any>();
 
 function makeDeps(
@@ -24,6 +25,7 @@ function makeDeps(
       }
     } as any,
     getChangeset: getChangesetMock,
+    getAncestorManifest: getAncestorManifestMock,
     core: { info: infoMock } as any,
     ...overrides
   };
@@ -45,6 +47,10 @@ describe('flagOverlappingOpenPrs', () => {
     listPullsMock.mockReset();
     createCommitStatusMock.mockReset().mockResolvedValue({});
     getChangesetMock.mockReset();
+    getAncestorManifestMock.mockReset().mockResolvedValue({
+      Button: 'h-button-old',
+      Modal: 'h-modal-old'
+    });
     infoMock.mockReset();
   });
 
@@ -222,4 +228,133 @@ describe('flagOverlappingOpenPrs', () => {
     expect(flagged.sort()).toEqual([200, 400]);
     expect(createCommitStatusMock).toHaveBeenCalledTimes(2);
   });
+  describe('stacked PRs', () => {
+    it('does not flag a PR whose baseline already held the merging values', async () => {
+      paginateMock.mockResolvedValue([
+        { number: 200, head: { sha: 'pr-200-head' } }
+      ]);
+      getChangesetMock.mockResolvedValue({
+        _headSha: 'merging-head',
+        Button: 'h-button-stacked'
+      } as Changeset);
+      getAncestorManifestMock.mockResolvedValue({ Button: 'h-button' });
+
+      const flagged = await flagOverlappingOpenPrs(
+        { bucket, repo, mergingPrNumber, mergingChangeset },
+        makeDeps()
+      );
+
+      expect(flagged).toEqual([]);
+      expect(getAncestorManifestMock).toHaveBeenCalledWith(
+        bucket,
+        'merging-head'
+      );
+      expect(createCommitStatusMock).not.toHaveBeenCalled();
+    });
+
+    it('treats a path the merging PR deletes as stacked when the baseline already lacked it', async () => {
+      paginateMock.mockResolvedValue([
+        { number: 200, head: { sha: 'pr-200-head' } }
+      ]);
+      getChangesetMock.mockResolvedValue({
+        _headSha: 'merging-head',
+        Modal: 'h-modal-readded'
+      } as Changeset);
+      getAncestorManifestMock.mockResolvedValue({ Button: 'h-button' });
+
+      const flagged = await flagOverlappingOpenPrs(
+        { bucket, repo, mergingPrNumber, mergingChangeset },
+        makeDeps()
+      );
+
+      expect(flagged).toEqual([]);
+      expect(createCommitStatusMock).not.toHaveBeenCalled();
+    });
+
+    it('still flags a stacked PR when the merging PR changed the path again after the stacked PR was compared', async () => {
+      paginateMock.mockResolvedValue([
+        { number: 200, head: { sha: 'pr-200-head' } }
+      ]);
+      getChangesetMock.mockResolvedValue({
+        _headSha: 'merging-head-earlier',
+        Button: 'h-button-stacked'
+      } as Changeset);
+      getAncestorManifestMock.mockResolvedValue({
+        Button: 'h-button-earlier'
+      });
+
+      const flagged = await flagOverlappingOpenPrs(
+        { bucket, repo, mergingPrNumber, mergingChangeset },
+        makeDeps()
+      );
+
+      expect(flagged).toEqual([200]);
+      expect(getAncestorManifestMock).toHaveBeenCalledWith(
+        bucket,
+        'merging-head-earlier'
+      );
+    });
+
+    it('flags when only some overlapping paths are stacked and reports just the stale ones', async () => {
+      paginateMock.mockResolvedValue([
+        { number: 200, head: { sha: 'pr-200-head' } }
+      ]);
+      getChangesetMock.mockResolvedValue({
+        _headSha: 'merging-head',
+        Button: 'h-button-stacked',
+        Modal: 'h-modal-other'
+      } as Changeset);
+      getAncestorManifestMock.mockResolvedValue({
+        Button: 'h-button',
+        Modal: 'h-modal-old'
+      });
+
+      const flagged = await flagOverlappingOpenPrs(
+        { bucket, repo, mergingPrNumber, mergingChangeset },
+        makeDeps()
+      );
+
+      expect(flagged).toEqual([200]);
+      const flagMessage = infoMock.mock.calls
+        .map((call: any[]) => call[0] as string)
+        .find((message: string) => message.startsWith('Flagging PR #200'));
+      expect(flagMessage).toContain('Modal');
+      expect(flagMessage).not.toContain('Button');
+    });
+
+    it('flags without resolving a baseline when the open changeset has no _headSha', async () => {
+      paginateMock.mockResolvedValue([
+        { number: 200, head: { sha: 'pr-200-head' } }
+      ]);
+      getChangesetMock.mockResolvedValue({
+        Button: 'h-button-other'
+      } as Changeset);
+
+      const flagged = await flagOverlappingOpenPrs(
+        { bucket, repo, mergingPrNumber, mergingChangeset },
+        makeDeps()
+      );
+
+      expect(flagged).toEqual([200]);
+      expect(getAncestorManifestMock).not.toHaveBeenCalled();
+    });
+
+    it('does not resolve a baseline for PRs with no disagreeing overlap', async () => {
+      paginateMock.mockResolvedValue([
+        { number: 200, head: { sha: 'pr-200-head' } }
+      ]);
+      getChangesetMock.mockResolvedValue({
+        _headSha: 'sha',
+        Button: 'h-button',
+        OtherThing: 'h-other'
+      } as Changeset);
+
+      await flagOverlappingOpenPrs(
+        { bucket, repo, mergingPrNumber, mergingChangeset },
+        makeDeps()
+      );
+
+      expect(getAncestorManifestMock).not.toHaveBeenCalled();
+    });
+  });
 });

docs/docs/usage/updating-base-images.md

@@ -30,3 +30,9 @@ status to failing with a message asking you to rebase. Comparadise will not acce
 `Visual Regression` status on your commit is failing, because the screenshots you reviewed were compared against a
 baseline that no longer exists. Rebase onto the default branch and re-run the visual tests, then review and accept
 the fresh comparison.
+
+Stacked PRs are exempt. If your PR targets another PR's branch and both change the same screenshot, your
+comparison already ran against the other PR's version of that screenshot, so nothing you reviewed is invalidated when
+it merges. Comparadise checks the baseline your PR was compared against and only asks you to rebase when that baseline
+disagrees with what actually landed — for example, when the other PR changed the screenshot again after your last
+visual test run.