fix: scope getStaticProps revalidate parsing to the exported function

[Boyeep]

1. Summary

This fixes a false positive in extractGetStaticPropsRevalidate().

Previously, the helper could match revalidate: anywhere in the file, including unrelated config objects outside getStaticProps. That could misclassify Pages Router routes in the build report.

This change scopes the search to the exported getStaticProps return object when that function is present.

2. Changes

• scope extractGetStaticPropsRevalidate() to getStaticProps
• add regression coverage for unrelated revalidate values outside getStaticProps
• add regression coverage showing the getStaticProps value still wins when both are present

3. Validation

• pnpm exec vp test run tests/build-report.test.ts
• pnpm exec vp fmt packages/vinext/src/build/report.ts tests/build-report.test.ts
• pnpm exec vp lint packages/vinext/src/build/report.ts tests/build-report.test.ts

#Notes

pnpm exec vp check reports existing repo-wide formatting issues in this checkout, so validation here was kept targeted to the changed files.

[james-elicx]

/bigbonk review

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@592

commit: 4280a13

[ask-bonk]

Good fix — the false-positive from matching revalidate: anywhere in the file was a real issue, and scoping the search to the getStaticProps return object is the right approach. The implementation is solid for a regex-based solution, the findMatchingBrace parser handles strings, comments, and nested braces correctly, and the fallback-to-whole-file behavior preserves backward compatibility for snippets without a getStaticProps declaration.

Two issues worth addressing, one medium-severity bug and one minor nit. The rest is clean.

[ask-bonk]

Bug (medium): First return wins, but early-return redirects are common.

declaration.search(/\breturn\s*\{/) matches the first return { in the function. If getStaticProps has an early return before the main props return, this extracts the wrong object:

export async function getStaticProps(ctx) {
  if (!ctx.params?.slug) {
    return { redirect: { destination: '/', permanent: false } }
  }
  return { props: { data }, revalidate: 60 }
}

Here the code extracts { redirect: { destination: '/', permanent: false } }, finds no revalidate: key, and returns null — missing the actual revalidate: 60 on the second return.

This is a common Next.js pattern (early-return redirect or notFound before the happy-path props return).

A simple fix: instead of taking the first return { match, search all return objects for revalidate: and take the first hit. Something like:

Suggested change

	const returnObjectStart =
	declaration.search(/\breturn\s*\{/) !== -1
	? declaration.search(/\breturn\s*\{/)
	: declaration.search(/=>\s*\(\s*\{/);
	if (returnObjectStart === -1) return declaration;
	const returnPattern = /\breturn\s*\{/g;
	let match;
	while ((match = returnPattern.exec(declaration)) !== null) {
	const braceStart = declaration.indexOf("{", match.index);
	if (braceStart === -1) continue;
	const braceEnd = findMatchingBrace(declaration, braceStart);
	if (braceEnd === -1) continue;
	const obj = declaration.slice(braceStart, braceEnd + 1);
	if (/\brevalidate\s*:/.test(obj)) return obj;
	}
	// Try arrow function implicit return: => ({ ... })
	const arrowMatch = declaration.search(/=>\s*\(\s*\{/);
	if (arrowMatch !== -1) {
	const braceStart = declaration.indexOf("{", arrowMatch);
	if (braceStart !== -1) {
	const braceEnd = findMatchingBrace(declaration, braceStart);
	if (braceEnd !== -1) return declaration.slice(braceStart, braceEnd + 1);
	}
	}
	return declaration;

This searches all return objects and returns the one that actually contains revalidate:, which handles the early-return-redirect pattern correctly.

A test case for this:

it("finds revalidate in second return when first return is a redirect", () => {
  const code = `export async function getStaticProps(ctx) {
  if (!ctx.params?.slug) {
    return { redirect: { destination: '/', permanent: false } }
  }
  return { props: { data: 1 }, revalidate: 60 }
}`;
  expect(extractGetStaticPropsRevalidate(code)).toBe(60);
});

[ask-bonk]

Nit: The backtick quote tracking works for simple template literals (`hello ${name}`), but nested template literals (`outer ${`inner`}`) would confuse the parser since it treats the inner backtick as closing the outer one.

Not worth fixing for this use case — getStaticProps return objects are almost always plain data, and this is a build report, not a runtime behavior change. Just noting it for the record.

[ask-bonk]

Nice test cases. Consider also adding a test for the early-return redirect pattern mentioned in the other comment — it's a common enough Next.js pattern that it's worth covering even if the fix comes in a follow-up.

[ask-bonk]

Review posted. Here's a summary of the feedback:

Overall: Good fix that addresses a real false-positive issue. The approach is sound — scoping the regex search to the getStaticProps return object, with a whole-file fallback for backward compatibility.

Issues raised:

1. Medium-severity bug: The code matches the first return { in the function, but early-return redirects/notFound are a very common Next.js pattern. If the first return is { redirect: ... } and a later return has revalidate: 60, the value is missed. I suggested iterating over all return {} objects and returning the first one that contains revalidate:, with a concrete code suggestion and test case.

2. Minor nit: The findMatchingBrace backtick handling doesn't support nested template literals, but this is practically irrelevant for getStaticProps return objects (plain data), so just noted for the record.

3. Test suggestion: Recommended adding a test for the early-return redirect pattern to cover the bug from point 1.

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Good fix for a real false-positive issue. The approach — scoping the regex to getStaticProps return objects with a whole-file fallback when no declaration is found — is the right one. The findMatchingBrace parser is solid, test coverage hits the important cases, and the early-return-redirect pattern is handled correctly.

One scoping bug worth fixing before merge (see inline comment), plus a minor suggestion.

[ask-bonk]

Bug: declaration includes everything after getStaticProps, not just its body.

code.slice(declarationMatch.index) captures from the start of getStaticProps to the end of the file. If another function is defined after getStaticProps and its return object happens to contain revalidate:, it gets picked up as a false positive — the exact class of bug this PR is trying to fix.

Example that would still produce a wrong result:

export function getStaticProps() {
  return { props: {} }
}

export function someHelper() {
  return { revalidate: 999 }
}

This would incorrectly return 999.

The fix is straightforward — you already have findMatchingBrace, so use it to find the closing brace of the function body and scope the search:

Suggested change

	const declaration = code.slice(declarationMatch.index);
	const fnBodyStart = code.indexOf("{", declarationMatch.index + declarationMatch[0].length);
	if (fnBodyStart === -1) return null;
	const fnBodyEnd = findMatchingBrace(code, fnBodyStart);
	if (fnBodyEnd === -1) return null;
	const declaration = code.slice(fnBodyStart, fnBodyEnd + 1);

This scopes declaration to just the function body of getStaticProps, so return statements in subsequent functions are excluded.

Note: for arrow functions assigned via const getStaticProps = async () => ({ ... }), the { after => may be a block body or the regex may not match the function keyword pattern. But the arrow function path (lines 158-167) already handles that separately, so this should be fine for the function keyword path.

[ask-bonk]

Good test cases. Consider adding a test for the scoping issue mentioned in the other comment — a function defined after getStaticProps that contains return { revalidate: ... }:

it("ignores revalidate in a function defined after getStaticProps", () => {
  const code = `export function getStaticProps() {
  return { props: {} };
}

export function unrelated() {
  return { revalidate: 999 };
}`;
  expect(extractGetStaticPropsRevalidate(code)).toBeNull();
});

This is the symmetric counterpart to the existing "ignores unrelated revalidate values outside getStaticProps" test (which covers values before the declaration). Having both ensures the scoping works in both directions.

[ask-bonk]

Minor: this empty-array return is a bit subtle. When getStaticProps is declared via the function keyword but has no return { ... } statements, returnObjects is empty so we fall through to the arrow function check. If that also fails, we return [] (truthy), which means the caller skips the whole-file fallback and returns null.

The behavior is correct — if we found a getStaticProps declaration, we should scope to it and not fall back. But the distinction between null ("no declaration found, use fallback") and [] ("declaration found but no return objects") is implicit. A brief comment here would help future readers understand the contract:

Suggested change

	if (arrowMatch === -1) return [];
	// getStaticProps was found but contains no return objects — return empty
	// (non-null signals the caller to skip the whole-file fallback).
	const arrowMatch = declaration.search(/=>\s*\(\s*\{/);
	if (arrowMatch === -1) return [];

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Overall: Good fix that addresses a real false-positive bug. The approach is sound — scoping the regex search to getStaticProps return objects with a whole-file fallback for backward compatibility. The findMatchingBrace parser is well-implemented, and the test coverage covers the key cases including early-return redirects.

Issues raised:

1. Scoping bug (main issue): code.slice(declarationMatch.index) captures everything from getStaticProps to end-of-file, not just the function body. A function defined after getStaticProps with return { revalidate: ... } would still produce a false positive — the exact class of bug this PR is fixing. The fix is simple: use the already-available findMatchingBrace to scope declaration to just the function body. Included a concrete suggestion block.

2. Test suggestion: Add a test with a function defined after getStaticProps containing a revalidate return, to complement the existing test for values before the declaration.

3. Minor clarity nit: The null vs [] return contract in extractGetStaticPropsReturnObjects is implicit. Suggested adding a comment to clarify the distinction for future readers.

github run

[james-elicx]

/bigbonk re-review

[Boyeep]

I did another hardening pass and pushed follow-up fixes for a few more realistic getStaticProps shapes.

In short, this now:

1. scopes parsing to the actual getStaticProps declaration
2. adds coverage for unrelated functions defined after getStaticProps
3. handles destructured params in function declarations and function expressions
4. ignores nested helper returns inside getStaticProps
5. avoids false positives for files that re-export getStaticProps from another module

I also added regression tests for each of those cases. Focused validation is still green with:

• pnpm exec vp test run tests/build-report.test.ts
• pnpm exec vp fmt packages/vinext/src/build/report.ts tests/build-report.test.ts
• pnpm exec vp lint packages/vinext/src/build/report.ts tests/build-report.test.ts

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk re-review

[james-elicx]

/bigbonk re-review

[ask-bonk]

Good fix for a real false-positive bug — scoping the revalidate search to the getStaticProps body is the right approach, and the test coverage is solid. The findMatchingBrace/findMatchingParen parsers are well-implemented, and handling early-return redirects, re-exports, destructured params, and nested helpers are all good additions.

One bug in the nested-body skipping logic that can produce false positives in a specific (but realistic) pattern. Details inline.

[ask-bonk]

Bug: nested function body skip undershoots — can produce false positives.

extractFunctionBody(code, i) returns code.slice(bodyStart, bodyEnd + 1) — only the body from { to }, starting at bodyStart. But i is at the f of function, which is earlier. i += nestedBody.length - 1 advances by the body's length, not by the distance from i to bodyEnd. Since bodyStart > i, the skip doesn't reach the end of the nested function.

Concrete example that triggers this:

export function getStaticProps() {
  function helper(longParam) {
    return { revalidate: 999 };
  }
  return { props: {} };
}

When scanning, i lands on function helper. extractFunctionBody returns the body { return { revalidate: 999 }; } (length ~31). i += 30 advances past function helper but lands back inside (longParam) { return { revalidate: 999 }; }, causing the inner return { revalidate: 999 } to be picked up as a top-level return.

The fix: compute the actual end position relative to i.

Suggested change

	i += nestedBody.length - 1;
	if (matchesKeywordAt(code, i, "function")) {
	const paramsStart = code.indexOf("(", i + "function".length);
	if (paramsStart !== -1) {
	const paramsEnd = findMatchingParen(code, paramsStart);
	if (paramsEnd !== -1) {
	const bodyStart = code.indexOf("{", paramsEnd + 1);
	if (bodyStart !== -1) {
	const bodyEnd = findMatchingBrace(code, bodyStart);
	if (bodyEnd !== -1) {
	i = bodyEnd;
	continue;
	}
	}
	}
	}
	continue;
	}

Alternatively, you could change extractFunctionBody to return the end index instead of the string, or return both. The same issue affects the class and arrow function skip paths below (lines 287-288, 295-296), though the arrow function gap is smaller and less likely to cause problems in practice.

[ask-bonk]

Same skip-undershoot bug as the function case above. extractClassBody(code, i) returns code.slice(bodyStart, bodyEnd + 1) where bodyStart is the { after the class name/extends clause. i is at class, so i += classBody.length - 1 doesn't reach bodyEnd.

Fix the same way — compute the actual end position:

Suggested change

	i += classBody.length - 1;
	if (matchesKeywordAt(code, i, "class")) {
	const bodyStart = code.indexOf("{", i + "class".length);
	if (bodyStart !== -1) {
	const bodyEnd = findMatchingBrace(code, bodyStart);
	if (bodyEnd !== -1) {
	i = bodyEnd;
	continue;
	}
	}
	continue;
	}

[ask-bonk]

Same category of bug, though the impact is smaller. i is at = of =>, extractArrowFunctionBody starts from arrowIndex + 2 (the character after >). The gap is only whitespace between > and {, so in practice this is unlikely to cause issues. But for correctness:

Suggested change

	i += nestedBody.length - 1;
	const nestedBody = extractArrowFunctionBody(code, i);
	if (nestedBody !== null) {
	const bodyStart = findNextNonWhitespaceIndex(code, i + 2);
	if (bodyStart !== -1) {
	i = bodyStart + nestedBody.length - 1;
	}
	}

Or inline the logic like the other two paths.

[ask-bonk]

The test passes because the helper uses an arrow function (small gap between => and {). Consider adding a test with a named nested function with parameters to cover the skip bug described in the other comment:

it("ignores revalidate in a nested named function inside getStaticProps", () => {
  const code = `export function getStaticProps() {
  function helper(longParam) {
    return { revalidate: 999 };
  }
  return { props: {} };
}`;
  expect(extractGetStaticPropsRevalidate(code)).toBeNull();
});

This would catch the skip-undershoot bug and guard against regressions.

[ask-bonk]

Review posted. Here's a summary of the feedback:

Overall: Good fix that addresses a real false-positive issue. The approach — scoping the regex search to the getStaticProps body using brace-matching parsers — is sound. Test coverage is solid, covering early-return redirects, re-exports, destructured params, nested helpers, and functions defined after getStaticProps.

Issue raised (one bug, three locations):

Bug: nested body skip undershoots in collectReturnObjectsFromFunctionBody. The function, class, and arrow function skip paths all use i += nestedBody.length - 1, but nestedBody is a substring starting from the body's opening {, not from position i. Since i points to the keyword (e.g., function) which is before the body's {, the skip doesn't advance far enough.

This is exploitable with a nested named function that has parameters:

export function getStaticProps() {
  function helper(longParam) {
    return { revalidate: 999 };
  }
  return { props: {} };
}

The existing test for nested helpers passes only because it uses an arrow function (minimal gap between => and {). Suggested fixing all three skip paths to compute the actual end position, and adding a test case for the named-function variant.

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Good fix. The false-positive from matching revalidate: anywhere in the file was a real issue, and scoping the search to the getStaticProps body using structured brace-matching is the right approach.

The refactoring from string-length-based skipping to find*BodyEnd functions that return absolute indices resolves the skip-undershoot concern from the earlier reviews — the nested named function test (line 234 in the test file) passes correctly because findFunctionBodyEnd returns the actual } index in the source, and i = nestedBodyEnd jumps directly to it.

Tests are comprehensive and all 72 pass. A few observations inline — one real (though low-probability) scoping issue in extractGetStaticPropsDeclaration, and some minor nits.

[ask-bonk]

Nit: re (line 113) is now only used in the fallback path (lines 124-128), but it's declared before the early-return on line 121. Consider inlining the regex at the use site to avoid constructing it when the scoped path handles everything:

const m = /\brevalidate\s*:\s*(-?\d+(?:\.\d+)?|Infinity|false)\b/.exec(code);

Minor clarity improvement — the current code works fine.

[ask-bonk]

Low-probability scoping issue: When declarationMatch is a const/let/var declaration (not a function keyword), declarationTail is everything from the match to end-of-file. The regex /(?:async\s+)?function\b/ on line 285 searches the entire tail.

If the actual assignment isn't a function expression — e.g. const getStaticProps = createGSP(options); — this regex will match the next function keyword anywhere later in the file, potentially scoping to an unrelated function body.

Very unlikely for getStaticProps in practice (it's almost always a function declaration, expression, or arrow). But if you want to tighten it, you could check that the matched function appears before any statement-terminating ; or newline after the = sign.

Not a blocker.

[ask-bonk]

Minor: findObjectMethodBodyEnd is called on every character that doesn't match a prior condition. It does keyword matching, name scanning, and paren/brace matching internally. For this use case (build report, run once per page file at build time) the cost is negligible, but if this parser is ever reused in a hot path, adding a guard like if (isIdentStart(code.charCodeAt(i)) || code[i] === '*') before the call would help.

[ask-bonk]

Good test coverage. The existing tests above this block (lines 148-161 — revalidate: 0, false, Infinity) use bare return statements without a getStaticProps wrapper, so they exercise the whole-file fallback path (lines 124-128). That's fine and intentional, but a brief comment noting they test the fallback would help readers understand why they don't wrap in getStaticProps while the new tests do.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Overall: Good fix that addresses a real false-positive issue. The approach — scoping the regex search to the getStaticProps body using structured brace-matching — is correct. All 72 tests pass.

Key observation: The skip-undershoot bug flagged in earlier reviews has been resolved in this revision. The code now uses find*BodyEnd functions that return absolute indices, and i = nestedBodyEnd jumps directly to the right position.

Issues raised:

1. Low-probability scoping issue (line 288): When getStaticProps is declared via const/let/var but assigned something other than a function expression (e.g., createGSP(options)), the function-expression regex search could match an unrelated function keyword later in the file. Very unlikely for getStaticProps in practice, not a blocker.

2. Minor nit (line 114): The re regex is now only used in the fallback path but is constructed unconditionally. Could be inlined at the use site.

3. Performance note (line 402): findObjectMethodBodyEnd is called on every unmatched character in the loop. Negligible for build-time use, but worth a guard if the parser is ever reused in a hot path.

4. Test clarity (line 170): The older tests that use bare return without a getStaticProps wrapper exercise the fallback path — a brief comment noting this would help future readers.

github run