Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
8 changes: 8 additions & 0 deletions .loopover.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,14 @@ review:
# drag_to: ".done-column" # Drag DESTINATION selector. Required when action is "drag"; an entry
# # missing it is dropped at parse time. Ignored for hover/click.
# label: "Reorder card into Done"
# # Zero-configuration alternative to interactions above: capture a hover-interaction GIF for any CSS
# # selector THIS PR'S OWN DIFF newly adds a `:hover`/`:focus-visible` rule for -- no maintainer selector-
# # authoring needed at all. Scoped to plain .css/.scss/.sass/.less files (a Tailwind utility class or
# # CSS-in-JS `:hover` state has no selector to extract this way). Composes with interactions above (a
# # manually-configured selector always wins over the same one auto-detected); both draw from the SAME
# # 3-per-PR cap. Bool. Default: false (byte-identical to today, no auto-detection). SELF-HOST ONLY (same
# # gate as interactions/gif above).
# auto_detect_interactions: true
# # Config-as-code enable/disable for this repo, layered ON TOP OF (never a replacement for) the
# # LOOPOVER_REVIEW_SCREENSHOTS + per-repo cutover-allowlist env-var gate above (#4083). Bool or null.
# # Default: null (unset) ⇒ defers entirely to that env-var gate's own decision -- byte-identical to today.
Expand Down
8 changes: 8 additions & 0 deletions config/examples/loopover.full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,14 @@ review:
# drag_to: ".done-column" # Drag DESTINATION selector. Required when action is "drag"; an entry
# # missing it is dropped at parse time. Ignored for hover/click.
# label: "Reorder card into Done"
# # Zero-configuration alternative to interactions above: capture a hover-interaction GIF for any CSS
# # selector THIS PR'S OWN DIFF newly adds a `:hover`/`:focus-visible` rule for -- no maintainer selector-
# # authoring needed at all. Scoped to plain .css/.scss/.sass/.less files (a Tailwind utility class or
# # CSS-in-JS `:hover` state has no selector to extract this way). Composes with interactions above (a
# # manually-configured selector always wins over the same one auto-detected); both draw from the SAME
# # 3-per-PR cap. Bool. Default: false (byte-identical to today, no auto-detection). SELF-HOST ONLY (same
# # gate as interactions/gif above).
# auto_detect_interactions: true
# # Config-as-code enable/disable for this repo, layered ON TOP OF (never a replacement for) the
# # LOOPOVER_REVIEW_SCREENSHOTS + per-repo cutover-allowlist env-var gate above (#4083). Bool or null.
# # Default: null (unset) ⇒ defers entirely to that env-var gate's own decision -- byte-identical to today.
Expand Down
15 changes: 13 additions & 2 deletions packages/loopover-engine/src/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,12 @@ export type VisualConfig = {
* (hover/click) — for behavior a static screenshot can't show that isn't scroll-linked (see `gif` above
* for scroll-linked evidence). Empty (default) ⇒ byte-identical to today, no interaction capture. */
interactions: VisualInteraction[];
/** `review.visual.autoDetectInteractions` (#auto-interaction-detection): capture a hover-interaction GIF
* for any CSS selector this PR's OWN diff newly adds a `:hover`/`:focus-visible` rule for — zero
* maintainer selector-authoring required, unlike `interactions` above (still available for a hand-
* curated demonstration; the two compose, deduped against each other). false (default) ⇒ byte-identical
* to today. Self-host only, same gate as `interactions`/`gif` (isScrollGifAvailable). */
autoDetectInteractions: boolean;
};

/** A `prefers-color-scheme` value the capture pipeline can emulate before rendering (#3678). */
Expand Down Expand Up @@ -1088,6 +1094,7 @@ export const EMPTY_VISUAL_CONFIG: VisualConfig = {
bugAnalysis: false,
bugAnalysisNotify: [],
interactions: [],
autoDetectInteractions: false,
};

/** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a
Expand Down Expand Up @@ -2996,6 +3003,7 @@ function overlayVisualConfig(base: VisualConfig, override: VisualConfig): Visual
bugAnalysis: override.bugAnalysis ? override.bugAnalysis : base.bugAnalysis,
bugAnalysisNotify: pickOverlayStringList(override.bugAnalysisNotify, base.bugAnalysisNotify),
interactions: override.interactions.length > 0 ? [...override.interactions] : [...base.interactions],
autoDetectInteractions: override.autoDetectInteractions ? override.autoDetectInteractions : base.autoDetectInteractions,
};
}

Expand Down Expand Up @@ -3203,7 +3211,8 @@ function visualConfigPresent(config: VisualConfig): boolean {
config.actionsFallback ||
config.bugAnalysis ||
config.bugAnalysisNotify.length > 0 ||
config.interactions.length > 0
config.interactions.length > 0 ||
config.autoDetectInteractions
);
}

Expand Down Expand Up @@ -3307,8 +3316,9 @@ function parseVisualConfig(value: JsonValue | undefined, warnings: string[]): Vi
const bugAnalysis = normalizeOptionalBoolean(record.bug_analysis, "review.visual.bug_analysis", warnings) === true;
const bugAnalysisNotify = parseVisualBugAnalysisNotify(record.bug_analysis_notify, warnings);
const interactions = parseVisualInteractions(record.interactions, warnings);
const autoDetectInteractions = normalizeOptionalBoolean(record.auto_detect_interactions, "review.visual.auto_detect_interactions", warnings) === true;

return { productionUrl, preview: { urlTemplate }, routes: { paths, maxRoutes }, themes, gif, enabled, themeStorageKey, actionsFallback, bugAnalysis, bugAnalysisNotify, interactions };
return { productionUrl, preview: { urlTemplate }, routes: { paths, maxRoutes }, themes, gif, enabled, themeStorageKey, actionsFallback, bugAnalysis, bugAnalysisNotify, interactions, autoDetectInteractions };
}

// A hard cap so a hostile/huge manifest can't turn every PR close into a giant @-mention blast — mirrors
Expand Down Expand Up @@ -3721,6 +3731,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue
return entry;
});
}
if (review.visual.autoDetectInteractions) visual.auto_detect_interactions = true;
out.visual = visual;
}
if (review.linkedIssueSatisfaction !== null) out.linkedIssueSatisfaction = review.linkedIssueSatisfaction;
Expand Down
9 changes: 8 additions & 1 deletion src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10384,6 +10384,13 @@ async function maybePublishPrPublicSurface(
const visualFiles = unifiedFiles
.map((file) => file.path)
.filter(isVisualPath);
// #auto-interaction-detection: only ever read by buildCapture when review.visual.autoDetectInteractions
// is on for this repo -- carries each changed file's own diff patch text (visualFiles above is bare
// paths), the same file.payload?.patch shape review-diff.ts/grounding-wire.ts already read elsewhere.
const changedCssFiles = unifiedFiles.map((file) => ({
path: file.path,
patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined,
}));
if (resolveConvergedFeature(env, repoFocusManifestForComment, "screenshots", repoFullName) && visualFiles.length > 0) {
try {
const token = await createInstallationToken(env, installationId);
Expand All @@ -10410,7 +10417,7 @@ async function maybePublishPrPublicSurface(
const capture =
reviewVisualConfig.enabled === false
? { routes: [], interactions: [], previewPending: false }
: await buildCapture(env, token, captureTarget, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), reviewVisualConfig);
: await buildCapture(env, token, captureTarget, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), reviewVisualConfig, changedCssFiles);
beforeAfter = capture.routes;
interactionPreviews = capture.interactions;
// Screenshot-table gate satisfaction (#4110): a successful capture (a real before+after render pair
Expand Down
45 changes: 43 additions & 2 deletions src/review/visual/capture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
import { captureInteractionFrames, captureScrollFrames, captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type InteractionAction, type ShotTheme, type Viewport } from "./shot";
import { compareCapturedScreenshots, isVisualDiffAvailable, type VisualDiffOutcome } from "./pixel-diff";
import { encodeScrollGif, isScrollGifAvailable } from "./scroll-gif";
import { detectAutoHoverInteractions, type ChangedCssFile } from "./interaction-detection";

const NAMESPACE = "loopover";
const DEFAULT_ROUTES = ["/"];
Expand Down Expand Up @@ -608,6 +609,12 @@ export type VisualCaptureConfig = {
* ⇒ byte-identical to today, no interaction capture. Capped at MAX_INTERACTIONS regardless of how many
* are configured. */
interactions?: readonly VisualInteractionInput[] | null | undefined;
/** `review.visual.autoDetectInteractions` (#auto-interaction-detection): capture a hover-interaction GIF
* for any CSS selector this PR's OWN diff newly adds a `:hover`/`:focus-visible` rule for — no maintainer
* selector-authoring needed, unlike `interactions` above (the two compose, deduped against each other).
* false/absent (default) ⇒ byte-identical to today. Requires `changedCssFiles` (below) to be passed too;
* without it there is nothing to detect against regardless of this flag. */
autoDetectInteractions?: boolean | null | undefined;
};

/**
Expand All @@ -616,7 +623,19 @@ export type VisualCaptureConfig = {
* collapsible). Fully fail-safe — a missing preview / failed render degrades to placeholders or dashes; this
* NEVER throws (the caller also wraps it in try/catch so a capture failure can't sink a review).
*/
export async function buildCapture(env: Env, token: string, target: CaptureTarget, visualFiles: string[], rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined, visualConfig?: VisualCaptureConfig | null | undefined): Promise<CaptureResult> {
export async function buildCapture(
env: Env,
token: string,
target: CaptureTarget,
visualFiles: string[],
rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined,
visualConfig?: VisualCaptureConfig | null | undefined,
// #auto-interaction-detection: the SAME changed-file set visualFiles is derived from, but carrying each
// file's diff patch text too (visualFiles alone is bare paths) -- only ever read when
// visualConfig.autoDetectInteractions is true. Absent/undefined (every pre-existing call site) ⇒
// byte-identical to today, no auto-detection attempted regardless of the config flag.
changedCssFiles?: readonly ChangedCssFile[] | undefined,
): Promise<CaptureResult> {
const repo = parseRepo(target.repoFullName);
const apiVersion = "2022-11-28";
// before = production. review.visual.production_url (#3611 follow-up) ALWAYS wins when set -- PUBLIC_SITE_ORIGIN
Expand Down Expand Up @@ -791,7 +810,29 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge
// interaction target" shape. Gated on isScrollGifAvailable() (reused: the encode step is frame-source-
// agnostic, see scroll-gif.ts) since there is no point capturing frames this build can never assemble into
// a GIF -- self-host only, same as the scroll-GIF path above.
const interactionsConfigured = (visualConfig?.interactions ?? []).slice(0, MAX_INTERACTIONS);
const manualInteractions = visualConfig?.interactions ?? [];
// #auto-interaction-detection: a maintainer-configured selector always wins on overlap -- an explicit
// entry may carry a label/path/action the detector could never infer, so a hand-authored duplicate is
// dropped from the auto-detected set rather than the other way around. Both selector sets are compared
// case-insensitively, matching detectAutoHoverInteractions' own dedup.
const manualSelectors = new Set(manualInteractions.map((interaction) => interaction.selector.toLowerCase()));
const autoDetectedInteractions: VisualInteractionInput[] =
visualConfig?.autoDetectInteractions && changedCssFiles
? detectAutoHoverInteractions(changedCssFiles)
.filter((selector) => !manualSelectors.has(selector.toLowerCase()))
.map((selector) => ({
selector,
action: "hover" as const,
// captureRoutes[0] is unreachable-undefined by construction here, not a reachable false case:
// `themes` above is always at least `[undefined]` and `routes` (resolveVisualRoutes ->
// mapFilesToRoutes) always falls back to DEFAULT_ROUTES when nothing else resolves, so the
// routes x themes double loop above always pushes at least one entry -- noUncheckedIndexedAccess
// still requires the optional chaining at the type level.
/* v8 ignore next */
path: captureRoutes[0]?.path ?? null,
}))
: [];
const interactionsConfigured = [...manualInteractions, ...autoDetectedInteractions].slice(0, MAX_INTERACTIONS);
const interactionRoutes: CaptureInteractionRoute[] = [];
// Interactions aren't multiplied per-theme (see comment above) -- when review.visual.themes configures more
// than one, the first configured theme is what interaction GIFs render in; themes[0] is `undefined` by
Expand Down
73 changes: 73 additions & 0 deletions src/review/visual/interaction-detection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Automatic hover-interaction detection from CSS diffs (#auto-interaction-detection). PURE, no DB/network —
// mirrors visual-findings.ts's own "pure decision logic only" convention. The whole point of
// review.visual.interactions (capture.ts / shot.ts) was originally a maintainer hand-authoring CSS selectors
// ahead of time; that still exists for a maintainer-curated demonstration, but requires foreknowledge of
// what's interactive and worth showing. This module is the zero-configuration alternative: read the PR's own
// diff for a newly ADDED `:hover`/`:focus-visible` CSS rule and capture ITS selector automatically — no
// maintainer selector-authoring step at all. Scoped to plain CSS/SCSS/SASS/LESS stylesheets (the only case a
// selector is syntactically explicit in the diff text); a Tailwind utility class or CSS-in-JS `:hover` state
// has no equivalent selector to extract this way and is out of scope here.

/** One changed file's path + unified-diff patch text — the same `file.payload?.patch` shape every other
* diff-reading module in this codebase already uses (review-diff.ts, grounding-wire.ts, ...). `patch`
* absent (a binary file, or a diff GitHub didn't include) ⇒ that file contributes no selectors. */
export type ChangedCssFile = { path: string; patch?: string | undefined };

const CSS_FILE_EXTENSIONS = [".css", ".scss", ".sass", ".less"];

// Mirrors capture.ts's MAX_INTERACTIONS reasoning: bounds how many auto-detected selectors this module ever
// returns, independent of how many `:hover`/`:focus-visible` rules a large stylesheet diff actually touches.
const MAX_AUTO_DETECTED_INTERACTIONS = 3;
// A selector this long is either a hostile/malformed diff line or a compound rule not worth interacting with
// (e.g. an entire multi-selector block) — mirrors focus-manifest.ts's MAX_ITEM_LENGTH-style bound.
const MAX_SELECTOR_LENGTH = 300;

// Matches a unified-diff ADDED line (`+`-prefixed, not the `+++` file-header line) whose CSS rule selector
// ends in `:hover` or `:focus-visible`, immediately followed by optional whitespace and the rule's opening
// `{`. Capturing only ADDED lines is deliberate: an EXISTING :hover rule this PR never touched says nothing
// about what changed, and would fire this feature on every single PR that merely touches a stylesheet.
const HOVER_SELECTOR_LINE_PATTERN = /^\+(?!\+\+)\s*([^{}\n]+?):(?:hover|focus-visible)\s*\{/;

function isCssFile(path: string): boolean {
const lower = path.toLowerCase();
return CSS_FILE_EXTENSIONS.some((ext) => lower.endsWith(ext));
}

/** The regex's own capture group spans from the line start to the LAST `:hover`/`:focus-visible` it found
* (non-greedy backtracking) — for a comma-separated selector LIST (`.a:hover, .b:hover { ... }`), that
* swallows every earlier selector's OWN `:hover` mid-string too (`.a:hover, .b`), not just `.b`. Since the
* match only anchors on the FINAL `:hover`/`:focus-visible` in the list, the text after the last comma is
* always the one real selector that rule actually matched against — take that, discarding the earlier
* list entries this capture can't cleanly separate rather than returning a mangled, unusable string. */
function lastSelectorInList(capturedGroup: string): string {
const lastCommaIndex = capturedGroup.lastIndexOf(",");
return (lastCommaIndex === -1 ? capturedGroup : capturedGroup.slice(lastCommaIndex + 1)).trim();
}

/**
* Detect newly-added `:hover`/`:focus-visible` CSS selectors across `files`' diff patches, capped at
* {@link MAX_AUTO_DETECTED_INTERACTIONS} and deduped case-insensitively. Selectors are returned in
* first-seen order (the order their files appear in `files`, then line order within each patch) — the
* caller decides what page/theme to capture them against. An unparseable/absent patch, a non-CSS file, or a
* selector exceeding {@link MAX_SELECTOR_LENGTH} contributes nothing; this NEVER throws.
*/
export function detectAutoHoverInteractions(files: readonly ChangedCssFile[]): string[] {
const selectors: string[] = [];
const seen = new Set<string>();
for (const file of files) {
if (selectors.length >= MAX_AUTO_DETECTED_INTERACTIONS) break;
if (!isCssFile(file.path) || !file.patch) continue;
for (const line of file.patch.split("\n")) {
if (selectors.length >= MAX_AUTO_DETECTED_INTERACTIONS) break;
const match = HOVER_SELECTOR_LINE_PATTERN.exec(line);
if (!match) continue;
const selector = lastSelectorInList(match[1]!);
if (!selector || selector.length > MAX_SELECTOR_LENGTH) continue;
const key = selector.toLowerCase();
if (seen.has(key)) continue;
seen.add(key);
selectors.push(selector);
}
}
return selectors;
}
Loading
Loading