Skip to content

fix(review): narrow visual screenshot path gate #4648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JSONbored wants to merge 1 commit into from
Closed

fix(review): narrow visual screenshot path gate #4648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions src/review/visual/paths.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
// Visual-path classifier (reviewbot→gittensory convergence — visual capture port).
//
// PORTED VERBATIM from reviewbot's src/agents/gittensory/capabilities.ts `isVisualPath` (the three
// VISUAL_PATTERNS), with the first pattern's app-folder segment widened to a wildcard (#3611 follow-up) so it
// isn't gittensory-ui-only — see capture.ts's DEFAULT_ROUTE_FILE for the same generalization. This is the
// EMPHATIC gate: screenshots fire ONLY for WEB-VISIBLE changes — any frontend app folder (apps/*/**, e.g.
// apps/gittensory-ui/** or apps/ui/**), a public asset (public/**, e.g. an OG image), or a front-of-house
// source extension (.tsx/.jsx/.css/.scss/.sass/.less/.html/.svg/.astro/.vue/.svelte/.mdx). A backend change
// VISUAL_PATTERNS), keeping capture scoped to file types and public assets that are directly web-visible.
// Route inference is still generalized in capture.ts's DEFAULT_ROUTE_FILE, but this cost gate must not treat
// every apps/* file as visual: screenshot-enabled repos often keep backend/config/docs files there too. This is
// the EMPHATIC gate: screenshots fire ONLY for WEB-VISIBLE changes — a public asset (public/**, e.g. an OG
// image), or a front-of-house source extension
// (.tsx/.jsx/.css/.scss/.sass/.less/.html/.svg/.astro/.vue/.svelte/.mdx). A backend change
// (.ts/.md/.json/.py/...) matches NONE of these, so capture never triggers for it.
//
// PURE — no imports, no I/O. Callers MUST filter changed files through this before any capture.

const VISUAL_PATTERNS: RegExp[] = [
/^apps\/[^/]+\//i,
/(^|\/)public\//i,
/\.(tsx|jsx|css|scss|sass|less|html|svg|astro|vue|svelte|mdx)$/i,
];
Expand Down
16 changes: 5 additions & 11 deletions test/unit/visual-paths.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,10 @@ import { describe, expect, it } from "vitest";
import { isVisualPath } from "../../src/review/visual/paths";

describe("isVisualPath (web-visible-only capture gate)", () => {
it("matches frontend app paths (apps/gittensory-ui/**)", () => {
it("matches frontend route files by web-visible extension, including generalized apps/* route folders", () => {
expect(isVisualPath("apps/gittensory-ui/src/routes/index.tsx")).toBe(true);
expect(isVisualPath("apps/gittensory-ui/src/routes/app.analytics.tsx")).toBe(true);
// Even a non-source file under the UI app is web-visible scope.
expect(isVisualPath("apps/gittensory-ui/public/og.png")).toBe(true);
expect(isVisualPath("apps/gittensory-ui/README.md")).toBe(true);
});

it("matches ANY app folder, not just gittensory-ui (#3611 follow-up — e.g. metagraphed's apps/ui/**)", () => {
expect(isVisualPath("apps/ui/src/routes/index.tsx")).toBe(true);
// Same non-extension-matching-file case as gittensory-ui above, now for a different app folder name.
expect(isVisualPath("apps/ui/components.json")).toBe(true);
expect(isVisualPath("apps/marketing-site/README.md")).toBe(true);
});

it("matches public asset paths (public/** — OG images etc.) at any depth", () => {
Expand Down Expand Up @@ -55,12 +46,15 @@ describe("isVisualPath (web-visible-only capture gate)", () => {
"Cargo.toml",
"src/data/seed.sql",
"config.yaml",
"apps/api/src/server.ts",
"apps/ui/components.json",
"apps/marketing-site/README.md",
]) {
expect(isVisualPath(path), path).toBe(false);
}
});

it("is case-insensitive on extensions and the app prefix", () => {
it("is case-insensitive on extensions", () => {
expect(isVisualPath("APPS/GITTENSORY-UI/src/Page.TSX")).toBe(true);
expect(isVisualPath("src/Icon.SVG")).toBe(true);
// A .ts (backend) must still be false even upper-cased — it is not a web-visible extension.
Expand Down