Skip to content

fix(nextjs): ignore response header mutations in GET handlers#209

Open
aqilaziz wants to merge 1 commit into
millionco:mainfrom
aqilaziz:fix-nextjs-get-header-false-positive
Open

fix(nextjs): ignore response header mutations in GET handlers#209
aqilaziz wants to merge 1 commit into
millionco:mainfrom
aqilaziz:fix-nextjs-get-header-false-positive

Conversation

@aqilaziz
Copy link
Copy Markdown

@aqilaziz aqilaziz commented May 12, 2026

Summary

Fixes #206.

Testing

  • pnpm exec vp test run tests/regressions/nextjs-side-effects.test.ts
  • pnpm exec tsc --noEmit from packages/react-doctor
  • pnpm exec vp lint
  • git diff --check

Note

Low Risk
Low risk: changes are limited to lint-side-effect heuristics and test fixtures, primarily reducing false positives without affecting runtime behavior.

Overview
Refines nextjs-no-side-effect-in-get-handler detection by teaching findSideEffect to ignore outbound response header shaping (e.g. response.headers.set/append/delete and headers().* mutations) and ignore mutations on request-scoped collections (new Headers/Map/Set created within the handler).

Adds a Next.js route fixture and a regression test ensuring these patterns no longer produce diagnostics, and includes a patch changeset for react-doctor.

Reviewed by Cursor Bugbot for commit bcde713. Bugbot is set up for automated code reviews on this repo. Configure here.

@reactreview
Copy link
Copy Markdown

reactreview Bot commented May 12, 2026

Note

No issues found

@vercel
Copy link
Copy Markdown

vercel Bot commented May 12, 2026

@aqilaziz is attempting to deploy a commit to the Million Team on Vercel.

A member of the Team first needs to authorize it.

@aqilaziz aqilaziz force-pushed the fix-nextjs-get-header-false-positive branch from 9598ea1 to 1ed2336 Compare May 12, 2026 00:51
}
if (object?.type !== "CallExpression" || object.callee?.type !== "Identifier") return false;
return object.callee.name === "headers";
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared findSideEffect suppresses headers() detection across frameworks

Medium Severity

isHeadersFunctionMutationCall matches headers().set/append/delete(...) and silently suppresses it before the existing isCookiesOrHeadersCall(child, "headers") check can flag it. Since findSideEffect is shared with the TanStack Start rule (tanstack-start.ts line 562), this suppression also applies there — where headers() is not the Next.js read-only accessor and could be a user-defined function returning a mutable object. The headers() branch of isCookiesOrHeadersCall is now effectively dead code for all real Headers API methods. The function also has zero test coverage — the test fixture only exercises isHeadersApiMutationCall and isRequestScopedMutationCall.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1ed2336. Configure here.

@aqilaziz aqilaziz force-pushed the fix-nextjs-get-header-false-positive branch from 1ed2336 to bcde713 Compare May 15, 2026 10:39
@aqilaziz
Copy link
Copy Markdown
Author

Rebased/rebuilt this PR onto current main after the monorepo refactor, so GitHub now reports it as mergeable.

What changed in the updated branch:

  • moved the fix to the new oxlint-plugin-react-doctor rule utility location
  • kept the same behavior: ignore response header shaping (response.headers.set/append/delete) and request-scoped Headers/Map/Set mutations in GET handlers
  • added a Next.js fixture route and regression assertion for the headers case

Verification:

  • pnpm -C packages/oxlint-plugin-react-doctor typecheck
  • built the internal packages on Windows using PowerShell env syntax for NODE_ENV
  • direct runOxlint check against the new app/headers/route.tsx fixture returns no nextjs-no-side-effect-in-get-handler diagnostics

Note: pnpm -C packages/react-doctor test -- tests/run-oxlint/nextjs.test.ts currently still has unrelated baseline failures on current main in this checkout, including existing Next.js rule expectations. Vercel is also blocked by team authorization as before.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

There are 3 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit bcde713. Configure here.

return false;
if (!isNodeOfType(object, "CallExpression")) return false;
return isNodeOfType(object.callee, "Identifier") && object.callee.name === "headers";
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isHeadersFunctionMutationCall silently suppresses Next.js headers() side-effect detection

High Severity

isHeadersFunctionMutationCall matches the exact same AST pattern as isCookiesOrHeadersCall(child, "headers") for methods set, append, and delete. Both detect headers().set(...) where headers is a direct function call — which is the Next.js headers() API from next/headers. Because the new skip on line 121 runs before the existing side-effect detection on lines 128–131, calls like headers().set() in GET handlers are now silently ignored instead of being flagged as side effects. The PR intended to suppress only response header mutations (response.headers.set()), not the Next.js request-headers API.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bcde713. Configure here.

bindings.add(child.id.name);
});
return bindings;
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope-blind binding collection causes false suppression across scopes

Low Severity

collectRequestScopedMutationBindings uses walkAst to recurse into the entire handler body — including nested functions and callbacks — collecting binding names into a flat Set without any scope information. If a nested function declares const store = new Map(), the name "store" is added globally. Then isRequestScopedMutationCall will incorrectly suppress an outer-scope store.set(...) where store refers to a completely different binding (e.g., an external KV store), causing a real side effect to go undetected.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bcde713. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False positive: nextjs-no-side-effect-in-get-handler flags Response.headers.set() and similar Headers API calls

1 participant