[No merge][NoQA] Test suite for "Convert code-inline-reviewer rules into proactive coding guidelines for Claude"#82206
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
kacper-mikolajczak
changed the title
melvin-bot
bot
requested review from
JmillsExpensify and
MonilBhavsar
and removed request for
a team
|
@MonilBhavsar Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
1 similar comment
|
@MonilBhavsar Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@JmillsExpensify @MonilBhavsar please disregard the reviewer request - this is a synthetic testing PR for AI reviewer improvement. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| function MyCard(_props: {transaction: Transaction; onPress: () => void}) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-1 (docs)
The spread operator {...item} creates a new object on every render iteration, forcing React to treat each item as changed and preventing reconciliation optimizations.
Fix: Pass props individually instead of spreading:
const renderItem = ({item}: {item: Item}) => <ItemRow id={item.id} reportID={item.reportID} value={item.value} highlight />;Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| const renderItem = ({item}: {item: Item}) => <ItemRow {...item} highlight />; | ||
|
|
||
| function ExpensiveComponent({reportID}: {reportID?: string}) { |
There was a problem hiding this comment.
❌ PERF-2 (docs)
Expensive computation computeExpensiveLayout(data) occurs before validating reportID. If reportID is undefined, the expensive work is wasted.
Fix: Move the validation check before expensive operations:
function ExpensiveComponent({reportID}: {reportID?: string}) {
if (!reportID) {
return null;
}
const result = computeExpensiveLayout(data);
return <View>{result}</View>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return <View>{result}</View>; | ||
| } | ||
|
|
||
| function ListItem({reportID}: {reportID: string}) { |
There was a problem hiding this comment.
❌ PERF-3 (docs)
Using useOnyx in a component rendered inside renderItem creates separate subscriptions for each list item, causing memory overhead and update cascades.
Fix: Use OnyxListItemProvider hooks instead:
function ListItem({reportID}: {reportID: string}) {
const report = useReport(reportID);
return <Text>{report?.reportName}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); | ||
| return <Text>{report?.reportName}</Text>; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-5 (docs)
Using deepEqual for the entire transaction object creates performance overhead that often exceeds the re-render cost. Compare only specific relevant properties.
Fix: Use shallow comparison of specific properties:
const MemoizedCard = memo(MyCard, (prev, next) =>
prev.transaction.transactionID === next.transaction.transactionID &&
prev.onPress === next.onPress
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const MemoizedCard = memo(MyCard, (prev, next) => deepEqual(prev.transaction, next.transaction) && prev.onPress === next.onPress); | ||
|
|
||
| function Greeting({firstName, lastName}: Props) { | ||
| const [fullName, setFullName] = useState(''); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Using useEffect to update state based on props causes an extra render pass and synchronization issues. The value can be computed directly.
Fix: Compute the value directly during rendering:
function Greeting({firstName, lastName}: Props) {
const fullName = `${firstName} ${lastName}`;
return <Text>{fullName}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function CommentForm({userId}: {userId: string}) { | ||
| const [comment, setComment] = useState(''); | ||
| const [rating, setRating] = useState(0); |
There was a problem hiding this comment.
❌ PERF-7 (docs)
Using useEffect to reset all component state when userId changes is not idiomatic. Use the key prop instead to trigger a full component reset.
Fix: Move the component reset to parent using key:
// In parent:
<CommentForm key={userId} userId={userId} />
// In component (no useEffect needed):
function CommentForm({userId}: {userId: string}) {
const [comment, setComment] = useState(');
const [rating, setRating] = useState(0);
// Component resets automatically when userId changes due to key prop
return <View>...</View>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| function BuyButton({onBuy}: {onBuy: () => void}) { | ||
| const [isBuying, setIsBuying] = useState(false); |
There was a problem hiding this comment.
❌ PERF-8 (docs)
Using useEffect to respond to the user event (isBuying state change) adds unnecessary render cycles. Handle the action directly in the event handler.
Fix: Call the callback directly in the event handler:
function BuyButton({onBuy}: {onBuy: () => void}) {
const handlePress = () => {
onBuy();
};
return <Pressable onPress={handlePress} />;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function ChainedEffects({firstName, lastName}: Props) { | ||
| const [fullName, setFullName] = useState(''); | ||
| const [isValid, setIsValid] = useState(false); |
There was a problem hiding this comment.
❌ PERF-9 (docs)
Multiple useEffect hooks form a chain where one effect's state update (setFullName) triggers another effect (setIsValid), creating unnecessary renders.
Fix: Compute derived values directly:
function ChainedEffects({firstName, lastName}: Props) {
const fullName = `${firstName} ${lastName}`;
const isValid = fullName.length > 0;
return (
<View>
<Text>{fullName}</Text>
<Text>{String(isValid)}</Text>
</View>
);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| ); | ||
| } | ||
|
|
||
| function Slider({value, onValueChange}: {value: number; onValueChange: (v: number) => void}) { |
There was a problem hiding this comment.
❌ PERF-10 (docs)
Using useEffect to call the parent callback onValueChange for communicating state changes violates React's unidirectional data flow pattern.
Fix: Lift state up to the parent component:
// In parent:
function Parent() {
const [value, setValue] = useState(0);
return <Slider value={value} onValueChange={setValue} />;
}
// In child:
function Slider({value, onValueChange}: {value: number; onValueChange: (v: number) => void}) {
return <input value={value} onChange={(e) => onValueChange(Number(e.target.value))} />;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| }, [internal, onValueChange]); | ||
| return <Text>{internal}</Text>; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-11 (docs)
Using the entire PERSONAL_DETAILS_LIST object without a selector causes re-renders when any unrelated field changes, even though only currentUser.displayName is used.
Fix: Use a selector to access only the needed data:
function UserDisplayName() {
const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {
selector: (details) => details?.currentUser?.displayName,
});
return <Text>{personalDetails}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| function Timer() { | ||
| const [count, setCount] = useState(0); |
There was a problem hiding this comment.
❌ PERF-12 (docs)
The setInterval is not cleared when the component unmounts, causing a memory leak and potential crashes in long-lived components.
Fix: Clear the interval in the cleanup function:
function Timer() {
const [count, setCount] = useState(0);
useEffect(() => {
const intervalId = setInterval(() => setCount((c) => c + 1), 1000);
return () => clearInterval(intervalId);
}, []);
return <Text>{count}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function ItemList({items}: {items: Item[]}) { | ||
| return ( | ||
| <View> |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The getTheme() function call doesn't use the iterator variable and executes redundantly on every iteration, creating O(n) overhead.
Fix: Hoist the iterator-independent call outside the loop:
function ItemList({items}: {items: Item[]}) {
const theme = getTheme();
return (
<View>
{items.map((item) => (
<Row key={item.id} color={theme.color} value={item.value} />
))}
</View>
);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| </View> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-14 (docs)
Using useEffect + setState to mirror window.innerWidth misses React's concurrent rendering guarantees. useSyncExternalStore is purpose-built for this pattern.
Fix: Use useSyncExternalStore:
import {useSyncExternalStore} from 'react';
function subscribe(callback: () => void) {
window.addEventListener('resize', callback);
return () => window.removeEventListener('resize', callback);
}
function WindowWidth() {
const width = useSyncExternalStore(
subscribe,
() => window.innerWidth,
() => 0 // server snapshot
);
return <Text>{width}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return <Text>{width}</Text>; | ||
| } | ||
|
|
||
| function SearchResults({query}: {query: string}) { |
There was a problem hiding this comment.
❌ PERF-15 (docs)
The async operation fetchResults lacks cleanup, allowing race conditions where a slow earlier response can overwrite a faster later response when query changes rapidly.
Fix: Add an ignore flag to discard stale responses:
function SearchResults({query}: {query: string}) {
const [results, setResults] = useState<never[]>([]);
useEffect(() => {
let ignore = false;
fetchResults(query).then((json) => {
if (!ignore) {
setResults(json);
}
});
return () => {
ignore = true;
};
}, [query]);
return <Text>{results.length}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| }, [query]); | ||
| return <Text>{results.length}</Text>; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-16 (docs)
Non-idempotent initialization logic (loadDataFromLocalStorage, checkAuthToken) will execute twice in React Strict Mode, potentially causing auth token invalidation or duplicate operations.
Fix: Add a module-level guard:
let didInit = false;
function AppInit() {
useEffect(() => {
if (didInit) return;
didInit = true;
loadDataFromLocalStorage();
checkAuthToken();
}, []);
return <View />;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Looks like this does not need my review. Feel free to request, if it changes |
|
|
||
| function DisplayName({first, last}: {first: string; last: string}) { | ||
| const [displayName, setDisplayName] = useState(''); | ||
| useEffect(() => { |
|
|
||
| function TotalDisplay({price, quantity}: {price: number; quantity: number}) { | ||
| const [total, setTotal] = useState(0); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
total is directly derivable from price * quantity — no state or effect needed. This causes an unnecessary extra render cycle on every prop change.
Compute the value inline instead:
function TotalDisplay({price, quantity}: {price: number; quantity: number}) {
const total = price * quantity;
return <Text>{total}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return null; | ||
| } | ||
|
|
||
| const renderItem = ({item}: {item: Item}) => <ItemRow {...item} highlight />; |
There was a problem hiding this comment.
❌ PERF-1 (docs)
Spreading ...item inside a renderItem function creates a new object on every render, preventing React reconciliation optimizations and causing unnecessary re-renders for every list item.
Pass individual props explicitly instead:
const renderItem = ({item}: {item: Item}) => (
<ItemRow id={item.id} reportID={item.reportID} value={item.value} highlight />
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const renderItem = ({item}: {item: Item}) => <ItemRow {...item} highlight />; | ||
|
|
||
| function ExpensiveComponent({reportID}: {reportID?: string}) { | ||
| const result = computeExpensiveLayout(data); |
There was a problem hiding this comment.
❌ PERF-2 (docs)
computeExpensiveLayout(data) executes before the !reportID guard check on line 59. If reportID is falsy, this expensive computation is wasted.
Move the early return before the expensive call:
function ExpensiveComponent({reportID}: {reportID?: string}) {
if (!reportID) {
return null;
}
const result = computeExpensiveLayout(data);
return <View>{result}</View>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return <Text>{report?.reportName}</Text>; | ||
| } | ||
|
|
||
| const MemoizedCard = memo(MyCard, (prev, next) => deepEqual(prev.transaction, next.transaction) && prev.onPress === next.onPress); |
There was a problem hiding this comment.
❌ PERF-5 (docs)
deepEqual(prev.transaction, next.transaction) in React.memo recursively compares all nested properties, creating performance overhead that often exceeds the re-render cost it aims to prevent.
Compare only the specific relevant properties instead:
const MemoizedCard = memo(MyCard, (prev, next) =>
prev.transaction.transactionID === next.transaction.transactionID &&
prev.onPress === next.onPress
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function Greeting({firstName, lastName}: Props) { | ||
| const [fullName, setFullName] = useState(''); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
fullName is directly derivable from firstName and lastName props — no state or effect needed. This causes an unnecessary extra render cycle on every prop change.
Compute the value inline instead:
function Greeting({firstName, lastName}: Props) {
const fullName = `${firstName} ${lastName}`;
return <Text>{fullName}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| function CommentForm({userId}: {userId: string}) { | ||
| const [comment, setComment] = useState(''); | ||
| const [rating, setRating] = useState(0); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-7 (docs)
This useEffect resets all component state (comment and rating) when userId changes. This is the exact pattern where the key prop should be used instead — React will unmount and remount the component, automatically resetting all state.
Use a key prop on the parent instead:
// In the parent component:
<CommentForm key={userId} userId={userId} />
// In CommentForm, remove the useEffect entirely:
function CommentForm({userId}: {userId: string}) {
const [comment, setComment] = useState("");
const [rating, setRating] = useState(0);
// State resets automatically when key changes
return (...);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function BuyButton({onBuy}: {onBuy: () => void}) { | ||
| const [isBuying, setIsBuying] = useState(false); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-8 (docs)
The buy action is a user event that should be handled directly in the event handler, not routed through state and a useEffect. This adds an unnecessary render cycle and makes the relationship between user action and response less clear.
Handle the event directly in the press handler:
function BuyButton({onBuy}: {onBuy: () => void}) {
return <Pressable onPress={() => onBuy()} />;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| function ChainedEffects({firstName, lastName}: Props) { | ||
| const [fullName, setFullName] = useState(''); | ||
| const [isValid, setIsValid] = useState(false); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-9 (docs)
These two useEffects form a chain: the first sets fullName, which triggers the second to set isValid. Both values are directly derivable from props and should be computed inline.
Compute derived values directly in the component body:
function ChainedEffects({firstName, lastName}: Props) {
const fullName = `${firstName} ${lastName}`;
const isValid = fullName.length > 0;
return (
<View>
<Text>{fullName}</Text>
<Text>{String(isValid)}</Text>
</View>
);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function Slider({value, onValueChange}: {value: number; onValueChange: (v: number) => void}) { | ||
| const [internal, setInternal] = useState(value); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-10 (docs)
This useEffect calls the parent callback onValueChange whenever internal state changes. Parent-child communication should not use useEffect. Instead, lift the state up to the parent and pass it down as props.
Lift state to the parent:
// Parent owns the state:
function Parent() {
const [value, setValue] = useState(0);
return <Slider value={value} onChange={setValue} />;
}
// Slider becomes a controlled component — no internal state or effect needed
function Slider({value, onChange}: {value: number; onChange: (v: number) => void}) {
return <Text>{value}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| function UserDisplayName() { | ||
| const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) subscribes to the entire personal details collection, but only currentUser?.displayName is used. The component will re-render whenever any field in the personal details list changes.
Use a selector to subscribe only to the needed data:
const [displayName] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {
selector: (details) => details?.currentUser?.displayName,
});
return <Text>{displayName}</Text>;Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function Timer() { | ||
| const [count, setCount] = useState(0); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-12 (docs)
setInterval is created without a cleanup function. The interval will continue running after the component unmounts, causing a memory leak and attempting to update state on an unmounted component.
Add a cleanup function to clear the interval:
useEffect(() => {
const intervalId = setInterval(() => setCount((c) => c + 1), 1000);
return () => clearInterval(intervalId);
}, []);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return ( | ||
| <View> | ||
| {items.map((item) => { | ||
| const theme = getTheme(); |
There was a problem hiding this comment.
❌ PERF-13 (docs)
getTheme() is called on every iteration of .map() but does not depend on the iterator variable item. It produces the same result each time, creating O(n) redundant calls.
Hoist the call outside the loop:
function ItemList({items}: {items: Item[]}) {
const theme = getTheme();
return (
<View>
{items.map((item) => (
<Row key={item.id} color={theme.color} value={item.value} />
))}
</View>
);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function WindowWidth() { | ||
| const [width, setWidth] = useState(window.innerWidth); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-14 (docs)
This useEffect manually subscribes to the resize DOM event and mirrors window.innerWidth into state via setWidth. This is the exact pattern useSyncExternalStore was designed for — it handles concurrent rendering edge cases and avoids the extra render pass.
Use useSyncExternalStore instead:
function subscribe(callback: () => void) {
window.addEventListener("resize", callback);
return () => window.removeEventListener("resize", callback);
}
function WindowWidth() {
const width = useSyncExternalStore(
subscribe,
() => window.innerWidth,
);
return <Text>{width}</Text>;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| function SearchResults({query}: {query: string}) { | ||
| const [results, setResults] = useState<never[]>([]); | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-15 (docs)
This useEffect fetches data based on query and calls setResults without any cleanup mechanism. If query changes rapidly, a slow earlier response can overwrite the result of a faster later response, showing stale data.
Add an ignore flag to discard stale responses:
useEffect(() => {
let ignore = false;
fetchResults(query).then((json) => {
if (\!ignore) {
setResults(json);
}
});
return () => { ignore = true; };
}, [query]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| function AppInit() { | ||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-16 (docs)
loadDataFromLocalStorage() and checkAuthToken() are non-idempotent initialization calls in a useEffect([]) without any execution guard. React Strict Mode will double-invoke this effect in development, potentially causing issues like invalidated tokens.
Add a module-level guard:
let didInit = false;
function AppInit() {
useEffect(() => {
if (didInit) {
return;
}
didInit = true;
loadDataFromLocalStorage();
checkAuthToken();
}, []);
return <View />;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| function PlatformButton() { | ||
| const isAndroid = Platform.OS === 'android'; |
There was a problem hiding this comment.
❌ CONSISTENCY-1 (docs)
Platform.OS === "android" is used to conditionally render different components inline. This mixes platform-specific logic within the component, increasing maintenance overhead and complexity.
Separate platform-specific implementations into dedicated files using file extensions:
PlatformButton.android.tsx // TouchableOpacity version
PlatformButton.ios.tsx // Pressable version
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| function RetryLogic() { | ||
| if (attempts < 3) { |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
3 and 5000 are magic numbers without documentation or named constants. Their purpose is unclear without context.
Extract them into named constants:
const MAX_RETRY_ATTEMPTS = 3;
const API_TIMEOUT_MS = 5000;
function RetryLogic() {
if (attempts < MAX_RETRY_ATTEMPTS) {
return apiCall({timeout: API_TIMEOUT_MS});
}
return null;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return null; | ||
| } | ||
|
|
||
| function formatCurrencyA(amount: number) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
formatCurrencyA and formatCurrencyB (line 204) are identical functions performing the same currency formatting. This violates the DRY principle and doubles maintenance burden.
Consolidate into a single reusable function:
function formatCurrency(amount: number) {
return new Intl.NumberFormat("en-US", {style: "currency", currency: "USD"}).format(amount);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return new Intl.NumberFormat('en-US', {style: 'currency', currency: 'USD'}).format(amount); | ||
| } | ||
|
|
||
| type BadProps = {title: string; onPress: () => void; unusedProp: string}; |
There was a problem hiding this comment.
❌ CONSISTENCY-4 (docs)
unusedProp is defined in BadProps but never used in the BadButton component. This increases component complexity and confuses the API surface.
Remove the unused prop from the type:
type BadProps = {title: string; onPress: () => void};Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| function UnjustifiedDisable() { | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
eslint-disable-next-line react-hooks/exhaustive-deps is used without an accompanying comment explaining why the rule is being disabled.
Add a justification comment:
// eslint-disable-next-line react-hooks/exhaustive-deps -- This effect should only run on mount; dependencies are intentionally omitted
useEffect(() => {
doSomething();
}, []);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return <View />; | ||
| } | ||
|
|
||
| async function submitExpense(expenseData: ExpenseData) { |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
API.submit(expenseData) is an async operation that can fail, but there is no error handling. If the submission fails, showSuccessMessage() would not execute but the error would be unhandled and the user would receive no feedback.
Add try/catch with proper error handling:
async function submitExpense(expenseData: ExpenseData) {
try {
await API.submit(expenseData);
showSuccessMessage();
} catch (error) {
Log.error("Expense submission failed", error);
showErrorMessage("Failed to submit expense. Please try again.");
}
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| showSuccessMessage(); | ||
| } | ||
|
|
||
| type ConfigTableProps = { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
ConfigTable uses boolean configuration props (shouldShowSearchBar, shouldShowHeader, shouldEnableSorting, shouldShowPagination) to control feature presence. Adding new features requires modifying this component API.
Use composition instead — express features as composable children:
<ConfigTable>
<ConfigTable.SearchBar />
<ConfigTable.Header sortable />
<ConfigTable.Body />
<ConfigTable.Pagination />
</ConfigTable>Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| ); | ||
| } | ||
|
|
||
| function ReportParent({reportID}: {reportID: string}) { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-2 (docs)
ReportParent fetches report, policy, and transaction via useOnyx only to pass them as props to ReportChild. The parent is a pure data intermediary — it does not use the data for its own rendering or logic. If ReportChild requirements change, the parent must change too.
Let ReportChild own its data by passing only the ID:
function ReportParent({reportID}: {reportID: string}) {
return <ReportChild reportID={reportID} />;
}
// ReportChild fetches its own data
function ReportChild({reportID}: {reportID: string}) {
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`);
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${report?.transactionID}`);
// ...
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| type WaypointListProps = { | ||
| transaction: Transaction; | ||
| navigateToWaypointEditPage: (index: number) => void; |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-3 (docs)
navigateToWaypointEditPage: (index: number) => void encodes a consumer-specific routing assumption into the component contract. The transaction prop is also passed primarily to compute the index for this callback, not for rendering purposes. This couples WaypointList to a specific navigation flow.
Use an abstract callback that describes what the component can do, not what the consumer should do:
type WaypointListProps = {
waypoints: Array<{id: string; name: string}>;
onSelectWaypoint: (waypoint: {id: string; name: string}) => void;
};
function WaypointList({waypoints, onSelectWaypoint}: WaypointListProps) {
return (
<View>
{waypoints.map((wp) => (
<Pressable key={wp.id} onPress={() => onSelectWaypoint(wp)}>
<Text>{wp.name}</Text>
</Pressable>
))}
</View>
);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| ); | ||
| } | ||
|
|
||
| function KitchenSink() { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-4 (docs)
KitchenSink aggregates five unrelated concerns in a single component: page view tracking, deep link handling, audio session setup, session checking, and navigation prefetching. Each concern is interleaved via separate useEffects, making it hard to modify one without risking regression in another.
Extract each concern into a focused custom hook or separate component:
function KitchenSink() {
usePageViewTracking();
useDeepLinkHandler();
useAudioSession();
useSessionCheck();
useNavigationPrefetch();
return <View />;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| const AppContext = React.createContext({}); | ||
| function AppProvider({children}: {children: React.ReactNode}) { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-5 (docs)
AppProvider bundles four unrelated Onyx subscriptions (priorityMode, chatReports, policies, transactions) into a single context value. Any consumer that only needs priorityMode will re-render when chatReports, policies, or transactions change — these are independent data domains that change at different frequencies.
Split into cohesive, focused contexts:
// Each provider owns a cohesive slice of state
function PriorityModeProvider({children}: {children: React.ReactNode}) {
const [priorityMode] = useOnyx(ONYXKEYS.NVP_PRIORITY_MODE);
return <PriorityModeContext.Provider value={priorityMode}>{children}</PriorityModeContext.Provider>;
}
// Or let consumers subscribe directly via useOnyx where neededPlease rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Product review not relevant on this one.
4 participants
Explanation of Change
The PR was created just to enable testing CI workflow of ai reviewer.
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari