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
13 changes: 13 additions & 0 deletions .changeset/quiet-wrapper-store-and-static-padding.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
"react-doctor": patch
"oxlint-plugin-react-doctor": patch
"eslint-plugin-react-doctor": patch
---

Stop `no-impure-state-updater` reporting callbacks handed to a helper that merely runs them (`run(async () => setValue("x"))`). Only a wrapper that forwards its parameter into a React setter's updater slot still counts as an updater.

Stop `nextjs-no-side-effect-in-get-handler` reporting `.set()` on a `Headers` object the helper constructs itself; mutations on stores the helper did not create still report.

Treat a ternary between static values (`hasHeader ? 0 : 16`) as static spacing in `rn-scrollview-dynamic-padding`, and reword its recommendation to name the matching `contentInset` edge and its iOS-only scope.

Accept a Zustand `store.setState(awaitedValue)` re-sync as a cache update in `query-mutation-missing-invalidation`; plain UI-state writes and non-store bindings still report.
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// verdict: pass
// rule: nextjs-no-side-effect-in-get-handler
// weakness: receiver-provenance
// source: GitHub issue #1808
// file-path: app/api/download/route.ts

declare function authorize(): Promise<Response | null>;

function downloadHeaders({
filename,
contentType,
contentLength,
}: {
filename: string;
contentType: string;
contentLength?: string | null;
}): Headers {
const headers = new Headers({
"Content-Type": contentType,
"Content-Disposition": `attachment; filename="${filename}"`,
});
if (contentLength) headers.set("Content-Length", contentLength);
return headers;
}

export async function GET() {
const denied = await authorize();
if (denied) return denied;
const upstream = await fetch("https://cdn.example.com/video.mp4");
return new Response(upstream.body, {
headers: downloadHeaders({ filename: "x", contentType: "video/mp4", contentLength: "123" }),
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// verdict: pass
// rule: no-impure-state-updater
// weakness: wrapper-transparency
// source: GitHub issue #1812
import { useCallback, useState } from "react";

export function Repro() {
const [busy, setBusy] = useState(false);
const [value, setValue] = useState("");
const run = useCallback(async (operation: () => Promise<void>) => {
setBusy(true);
try {
await operation();
} finally {
setBusy(false);
}
}, []);
const first = () =>
run(async () => {
await Promise.resolve();
setValue("first");
});
const second = () =>
run(async () => {
await Promise.resolve();
setValue("second");
});
return (
<button type="button" disabled={busy} onClick={value ? second : first}>
{value || "Run"}
</button>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// verdict: pass
// rule: query-mutation-missing-invalidation
// weakness: library-idiom
// source: GitHub issue #1786
import { useMutation } from "@tanstack/react-query";
import { Button, Text, View } from "react-native";
import { create } from "zustand";

declare function purchase(): Promise<"purchased" | "cancelled">;
declare function getMembership(): Promise<{ tier: string }>;
const useMembership = create<{ tier: string }>(() => ({ tier: "free" }));

async function reconcileMembership() {
const membership = await getMembership();
useMembership.setState(membership);
}

export function Upgrade() {
const tier = useMembership((state) => state.tier);
const upgrade = useMutation({
mutationFn: async () => {
if ((await purchase()) === "purchased") await reconcileMembership();
},
});
return (
<View>
<Text>{tier}</Text>
<Button title="Upgrade" disabled={upgrade.isPending} onPress={() => upgrade.mutate()} />
</View>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// verdict: fail
// rule: query-mutation-missing-invalidation
// weakness: library-idiom
// source: GitHub issue #1786 (counter-example: a UI-state store write is not a re-sync)
import { useMutation } from "@tanstack/react-query";
import { create } from "zustand";

declare const api: { archiveProject: (id: string) => Promise<void> };
const useUiStore = create(() => ({ isDialogOpen: false }));

export function useArchiveProject() {
return useMutation({
mutationFn: (id: string) => api.archiveProject(id),
onSuccess: () => {
useUiStore.setState({ isDialogOpen: false });
},
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// verdict: pass
// rule: rn-scrollview-dynamic-padding
// weakness: static-value-guard
// source: GitHub issue #1785
import type { ReactElement } from "react";
import { FlashList } from "@shopify/flash-list";
import { Text } from "react-native";

const rows = [{ id: "one" }];
function renderRow({ item }: { item: { id: string } }) {
return <Text>{item.id}</Text>;
}

function Grid({ ListHeaderComponent }: { ListHeaderComponent?: ReactElement }) {
return (
<FlashList
data={rows}
keyExtractor={(item) => item.id}
renderItem={renderRow}
ListHeaderComponent={ListHeaderComponent}
contentContainerStyle={{ paddingTop: ListHeaderComponent ? 0 : 16 }}
/>
);
}

export function Screen({ selectedTab }: { selectedTab: "profile" | "badges" }) {
return <Grid ListHeaderComponent={<Text>{selectedTab}</Text>} />;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ export const RENDER_PROP_PROLIFERATION_THRESHOLD = 3;
// `isMobile ? <Mobile /> : <Desktop />` switch is legitimate and stays quiet.
export const BOOLEAN_PROP_VARIANT_BRANCH_THRESHOLD = 2;
export const GET_HANDLER_BINDING_RESOLUTION_DEPTH = 3;
// Same-file wrapper hops `no-impure-state-updater` follows from a call
// site to the React setter that receives the forwarded updater
// (`update(fn)` → `(updater) => setCount(updater)`).
export const UPDATER_WRAPPER_RESOLUTION_DEPTH = 3;
export const SYNCHRONOUS_THROW_RESOLUTION_DEPTH = 3;
export const FUNCTION_RESOLUTION_MAX_DEPTH = 15;
// How many identifier→initializer hops jsx-key follows when proving a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12106,7 +12106,7 @@
"id": "rn-scrollview-dynamic-padding",
"title": "Dynamic padding on contentContainerStyle",
"severity": "warn",
"recommendation": "Use `contentInset={{ bottom: dynamicValue }}` so the OS shifts the content instead of relaying it out, which avoids the jump.",
"recommendation": "Move the changing value to the matching `contentInset` edge (`contentInset={{ bottom: keyboardHeight }}` for `paddingBottom`; iOS only) so the OS offsets the content instead of relaying it out, and keep static spacing in `contentContainerStyle`.",
"category": "Bugs",
"framework": "react-native",
"requires": ["react-native"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,58 @@ export async function GET(request) {
expect(result.diagnostics.length).toBeGreaterThan(0);
});

// Issue #1808: the helper constructs the response's own `Headers` and
// mutates it before returning — nothing outside the handler's return
// value changes, so a forged GET cannot observe anything.
it("stays silent on headers.set() over a Headers the helper itself constructed", () => {
const result = runRule(
nextjsNoSideEffectInGetHandler,
`function downloadHeaders({ filename, contentType, contentLength }: {
filename: string;
contentType: string;
contentLength?: string | null;
}): Headers {
const headers = new Headers({
"Content-Type": contentType,
"Content-Disposition": \`attachment; filename="\${filename}"\`,
});
if (contentLength) headers.set("Content-Length", contentLength);
return headers;
}

export async function GET(request: Request) {
const denied = await authorize();
if (denied) return denied;
const upstream = await fetch("https://cdn.example.com/video.mp4");
return new Response(upstream.body, {
headers: downloadHeaders({ filename: "x", contentType: "video/mp4", contentLength: "123" }),
});
}`,
{ filename: "app/api/download/route.ts" },
);
expect(result.parseErrors).toEqual([]);
expect(result.diagnostics).toEqual([]);
});

it("still flags a helper that mutates a store it did not construct", () => {
const result = runRule(
nextjsNoSideEffectInGetHandler,
`function touchSession(sessionStore) {
const headers = new Headers();
headers.set("Cache-Control", "no-store");
sessionStore.set("lastSeen", Date.now());
return headers;
}

export async function GET() {
return new Response(null, { headers: touchSession(kv) });
}`,
{ filename: "app/api/session/route.ts" },
);
expect(result.parseErrors).toEqual([]);
expect(result.diagnostics.length).toBeGreaterThan(0);
});

it("does not follow a second hop (helper calling another helper)", () => {
const result = runRule(
nextjsNoSideEffectInGetHandler,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,13 @@ export const nextjsNoSideEffectInGetHandler = defineRule({
helperFunction,
locallyScopedSafeBindings,
);
// The helper may build its own response object (`const headers =
// new Headers(...)`) — a `.set()` on that never leaves the
// helper's return value.
const effectiveSafeBindings = new Set([
...locallyScopedSafeBindings,
...helperParameterSafeBindings,
...collectLocallyScopedSafeBindings(helperBody),
]);

const sideEffectInHelper = findSideEffect(helperBody, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,61 @@ const C = () => <FlatList contentContainerStyle={{ paddingTop: spacing(4), paddi
expect(result.diagnostics.length).toBeGreaterThan(0);
});

// Issue #1785: a header-presence ternary between two literals.
it("stays silent on a ternary between static literals", () => {
const result = runRule(
rnScrollviewDynamicPadding,
`const Grid = ({ ListHeaderComponent }) => (
<FlashList
data={rows}
renderItem={renderRow}
ListHeaderComponent={ListHeaderComponent}
contentContainerStyle={{ paddingTop: ListHeaderComponent ? 0 : 16 }}
/>
);`,
);
expect(result.parseErrors).toEqual([]);
expect(result.diagnostics).toEqual([]);
});

it("stays silent on a ternary between static consts and a nested static ternary", () => {
const result = runRule(
rnScrollviewDynamicPadding,
`const HEADER_GAP = 0;
const DEFAULT_GAP = 16;
const C = ({ hasHeader, compact }) => (
<ScrollView
contentContainerStyle={{ paddingTop: compact ? 8 : hasHeader ? HEADER_GAP : DEFAULT_GAP }}
/>
);`,
);
expect(result.parseErrors).toEqual([]);
expect(result.diagnostics).toEqual([]);
});

it("still flags a ternary whose consequent tracks a live measurement", () => {
const result = runRule(
rnScrollviewDynamicPadding,
`const C = ({ isKeyboardOpen, keyboardHeight }) => (
<ScrollView contentContainerStyle={{ paddingBottom: isKeyboardOpen ? keyboardHeight : 0 }} />
);`,
);
expect(result.parseErrors).toEqual([]);
expect(result.diagnostics.length).toBeGreaterThan(0);
});

it("still flags a ternary whose alternate reads a dynamic inset", () => {
const result = runRule(
rnScrollviewDynamicPadding,
`const C = ({ hasTabBar }) => {
const insets = useSafeAreaInsets();
return <ScrollView contentContainerStyle={{ paddingBottom: hasTabBar ? 0 : insets.bottom }} />;
};`,
);
expect(result.parseErrors).toEqual([]);
expect(result.diagnostics.length).toBeGreaterThan(0);
});

it("stays silent on a negated static const", () => {
const result = runRule(
rnScrollviewDynamicPadding,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ const isStaticStyleValue = (value: EsTreeNode, resolutionDepth = 0): boolean =>
isStaticStyleValue(value.right, resolutionDepth + 1)
);
}
// A ternary that only picks between static values (`hasHeader ? 0 : 16`)
// is a discrete layout switch, not a value tracking a live measurement:
// the rows move once, with the structural change that flipped it, rather
// than on every keyboard or inset frame `contentInset` exists to absorb.
if (isNodeOfType(value, "ConditionalExpression")) {
return (
isStaticStyleValue(value.consequent, resolutionDepth + 1) &&
isStaticStyleValue(value.alternate, resolutionDepth + 1)
);
}
if (!isNodeOfType(value, "Identifier")) return false;
const binding = findVariableInitializer(value, value.name);
if (!binding?.initializer || !isConstDeclaredBinding(binding)) return false;
Expand All @@ -99,7 +109,7 @@ export const rnScrollviewDynamicPadding = defineRule({
requires: ["react-native"],
severity: "warn",
recommendation:
"Use `contentInset={{ bottom: dynamicValue }}` so the OS shifts the content instead of relaying it out, which avoids the jump.",
"Move the changing value to the matching `contentInset` edge (`contentInset={{ bottom: keyboardHeight }}` for `paddingBottom`; iOS only) so the OS offsets the content instead of relaying it out, and keep static spacing in `contentContainerStyle`.",
create: (context: RuleContext) => ({
JSXOpeningElement(node: EsTreeNodeOfType<"JSXOpeningElement">) {
const elementName = resolveJsxElementName(node);
Expand Down
Loading
Loading