[No QA] New AI PR reviewer prompt by jmusial · Pull Request #71655 · Expensify/App

jmusial · 2025-10-01T10:17:18Z

Explanation of Change

Updated Claude Reviewer prompt to test ONLY against provided rules.
AI prompt testing script. Allows to locally compare / review any two branches using claude CLI.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/521398

PROPOSAL:

Tests

Pre requisite:

must have claude CLI with at least pro plan. If you do not you can use --manual on the testing script and copy result to web claude.
chmod +x for .claude/utils/test-review-prompt.sh
Checkout the branch you want to test locally
Run ./test-review-prompt.sh main
See the output in the console

Offline tests

N/A

QA Steps

N/A

PR Author Checklist

Run against following branches with results:

Prompt testing

PR: Expensify card page uses navigation for magic code ✅

❌ [PERF-6]

Passing entire objects as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking, reducing unnecessary hook executions.

In src/pages/settings/Wallet/ExpensifyCardPage/ExpensifyCardContextProvider.tsx line 40, the useMemo depends on the entire setter functions instead of just the state values. The memoization dependencies should only include cardsDetails, isCardDetailsLoading, and cardsDetailsErrors:

const value = useMemo(
    () => ({
        cardsDetails,
        setCardsDetails,
        isCardDetailsLoading,
        setIsCardDetailsLoading,
        cardsDetailsErrors,
        setCardsDetailsErrors,
    }),
    [cardsDetails, isCardDetailsLoading, cardsDetailsErrors],
);

PR: Delay emoji modal opening when keyboard is closing ✅

LGTM . Thank you for your hard work!

PR: Navigation initialization console errors fix ✅

LGTM . Thank you for your hard work!

PR Feat/update ai review prompt - test branch with violations ✅

❌ [PERF-4] Memoize objects and functions passed as props

Line: 40-49 in src/components/SubStepForms/YesNoStep.tsx

Objects passed as props should be properly memoized to prevent unnecessary re-renders. The options array was previously memoized with useMemo but has been changed to a regular array that gets recreated on every render.

Reasoning: React uses referential equality to determine if props changed. Creating a new array instance on every render triggers unnecessary re-renders of child components, even when the actual data hasn't changed. The previous memoization preserved referential stability.

Fix: Restore the useMemo wrapper:

const options = useMemo<Choice[]>(
    () => [
        {
            label: translate('common.yes'),
            value: 'true',
        },
        {
            label: translate('common.no'),
            value: 'false',
        },
    ],
    [translate],
);

❌ [PERF-2] Use early returns in array iteration methods

Line: 71-98 in src/libs/PaginationUtils.ts

The code performs expensive operations (function calls like findFirstItem, findLastItem, array slicing, and mapping) before checking simple null conditions.

Reasoning: The null checks for firstItem and lastItem are simple property checks that can eliminate items early, but they're being performed after expensive operations have already executed. This wastes computation when items are null.

Fix: Perform null checks immediately after getting the items:

let firstItem = findFirstItem(sortedItems, page, getID);
let lastItem = findLastItem(sortedItems, page, getID);

// If all actions in the page are not found it will be removed.
if (firstItem === null || lastItem === null) {
    return null;
}

// In case actions were reordered, we need to swap them.
if (firstItem.index > lastItem.index) {
    const temp = firstItem;
    firstItem = lastItem;
    lastItem = temp;
}

const ids = sortedItems.slice(firstItem.index, lastItem.index + 1).map((item) => getID(item));
if (firstItem.id === CONST.PAGINATION_START_ID) {
    ids.unshift(CONST.PAGINATION_START_ID);
}
if (lastItem.id === CONST.PAGINATION_END_ID) {
    ids.push(CONST.PAGINATION_END_ID);
}

return {
    ids,
    firstID: firstItem.id,
    firstIndex: firstItem.index,
    lastID: lastItem.id,
    lastIndex: lastItem.index,
};

❌ [PERF-5] Use shallow comparisons instead of deep comparisons

Line: 324 in src/pages/iou/request/MoneyRequestAttendeeSelector.tsx

The memo comparison is using deepEqual(prevProps, nextProps) which performs a deep equality check on all props instead of comparing only specific relevant properties.

Reasoning: Deep equality checks recursively compare all nested properties, creating performance overhead that often exceeds the re-render cost they aim to prevent. Shallow comparisons of specific relevant properties provide the same optimization benefits with minimal computational cost.

Fix: Compare only specific relevant properties:

export default memo(MoneyRequestAttendeeSelector, (prevProps, nextProps) => 
    deepEqual(prevProps.attendees, nextProps.attendees) && 
    prevProps.iouType === nextProps.iouType
);

❌ [PERF-6] Use specific properties as hook dependencies

Line: 70 in src/pages/settings/Wallet/ReportVirtualCardFraudPage.tsx

The useEffect dependency array includes entire objects (formData, virtualCard, validateCodeAction) instead of specifying individual properties.

Reasoning: Passing entire objects as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking, reducing unnecessary hook executions and improving performance predictability.

Fix: Specify individual object properties as dependencies:

}, [formData?.isLoading, latestIssuedVirtualCardID, prevIsLoading, virtualCard?.errors, validateCodeAction?.errorFields]);

❌ [PERF-1] No spread in list item's renderItem

Line: 57-60 in tests/ui/ReportActionAvatarsTest.tsx

A spread operator is being used on an object (...rest) that is being passed as props within what appears to be a mock component's render function.

Reasoning: Creating new objects with spread operators in render contexts forces React to treat each render as changed, preventing reconciliation optimizations and causing unnecessary re-renders.

Fix: Pass individual properties explicitly:

return ({source, name, avatarID, testID = 'Avatar'}: AvatarProps) => {
    return (
        <MockedAvatarData
            dataSet={{
                name,
                avatarID,
                uri: parseSource(source ?? '') || 'No Source',
                parent: testID,
            }}
            testID="MockedAvatarData"
        />
    );
};

PR: Add F1 Custom Avatars SVGs ✅

LGTM . Thank you for your hard work!

PR: WIP - Add card fraud alert ✅

LGTM . Thank you for your hard work!

jmusial · 2025-10-01T10:25:55Z

.claude/agents/code-inline-reviewer.md

+
+**Conditions**: Flag ONLY when ALL of these are true:
+
+- Using .every(), .some(), .find(), .filter() or similar function


Updated PERF-1 & PERF-2 as I was getting inconsistent results. Checking if making rule more specific can help.

same here, let's not break the structure that is very strict and allows an llm to navigate more easily

jmusial · 2025-10-01T10:26:41Z

Claude still adds a short summary sometimes :( But it's non invasive so IMO should be fine to test with this version

melvin-bot · 2025-10-01T11:50:15Z

@brunovjk 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]

adhorodyski · 2025-10-01T12:05:42Z

.claude/utils/test-review-prompt.sh

+REVIEW_REQUEST="$REVIEW_OUTPUT_DIR/review-request-${TIMESTAMP}.txt"
+
+{
+    echo "Below you will see output for automated code review. For the purpose of this prompt: Instead of calling github actions output all comments to the console here, in the same format as if they were comments in GH. Follow the same rules as mentioned below, no unwanted output, only rules comments."


I'm not sure this helps us 'rate' the prompt in any way given how it can change the output:

Follow the same rules as mentioned below, no unwanted output, only rules comments.

I'd leave it to internal engineers if this is helpful as EApp's source code

ok, changed this a bit, removed parts with instruction from to not mention anything apart from "Run it locally" context + updated the actual prompt to be more liberal.

adhorodyski · 2025-10-01T12:06:48Z

.claude/utils/test-review-prompt.sh

+    echo "- Analyze the provided diff and file contents directly"
+    echo "- The files are ready for your review - proceed with the analysis"
+    echo ""
+    sed 's/Read each changed file carefully using the Read tool/Carefully analyze each changed file from the complete content provided below/g; s/using the available GitHub inline comment tool/as console output in GitHub comment format/g' "$PROMPT_FILE"


same as above, changes the overall prompt

adhorodyski · 2025-10-01T12:11:55Z

.claude/agents/code-inline-reviewer.md

   - `path`: Full file path (e.g., "src/components/ReportActionsList.tsx")
   - `line`: Line number where the issue occurs
   - `body`: Concise and actionable description of the violation and fix, following the below Comment Format
+4. **Each comment must reference exactly one Rule ID.**


encourage Claude to leave comments if uncertain

adhorodyski · 2025-10-03T14:16:27Z

.claude/agents/code-inline-reviewer.md

- **Condition**: When passing data to components in renderItem functions, avoid using spread operators to extend objects. Instead, pass the base object and additional properties as separate props to prevent unnecessary object creation on each render.
- **Reasoning**: `renderItem` functions execute for every visible list item on each render. Creating new objects with spread operators forces React to treat each item as changed, preventing reconciliation optimizations and causing unnecessary re-renders of child components.
+
+**Conditions**: Flag ONLY when ALL of these are true:


I'm afraid this addition breaks formatting of Conditions + Reasoning that is defined at the top. Do you see how we can break it down so both these are under this 'conditions' block?

Suggested change

**Conditions**: Flag ONLY when ALL of these are true:

**Conditions**:

Flag ONLY when ALL of these are true:

- a

- b

DO NOT flag if:

- c

- d

**Reasoning**: ...

adhorodyski · 2025-10-03T14:20:39Z

.claude/agents/code-inline-reviewer.md

+    If you found even ONE violation or have ANY uncertainty do NOT output LGTM - create inline comments instead.
+8. **DO NOT invent new rules, stylistic preferences, or commentary outside the listed rules.**
+9. **DO NOT describe what you are doing, output any summaries, explanations, extra content, comments on rules that are NOT violated or ANYTHING ELSE except from rules violations or LGTM message.**
+    EXCEPTION: If you believe something MIGHT be a Rule violation but are uncertain, err on the side of creating an inline comment with your concern rather than skipping it.


Can you give an example for that?

err on the side of creating an inline comment with your concern rather than skipping it.

The issue was that sometimes the prompt was not giving comments when there were clear violations. Sometimes is a keyword here as I was getting correct comments on one execution and then only some on another teminal for the same branch. adding EXCEPTION stabilized that

Mostly tested on PR: Expensify card page uses navigation for magic code.

adhorodyski · 2025-10-03T14:21:17Z

.claude/utils/test-review-prompt.sh

my only concern is if this belong to the source code

The idea behind adding it to provide consistent way to test prompts in the future. Can delete it though

I think we can propose some utils separately, with docs and more structure on the directory 👍🏼

mountiny · 2025-10-06T12:46:27Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified there are no new alerts related to the canBeMissing param for useOnyx
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

Android: mWeb Chrome

iOS: HybridApp

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

mountiny

All the changes look good to me, I think we do not have to wait much for other reviewers here as its iterative approach so lets test this and improve on it

OSBotify · 2025-10-06T13:01:45Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2025-10-06T20:12:16Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.24-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

OSBotify · 2025-10-07T15:19:38Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.26-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

OSBotify · 2025-10-08T04:51:09Z

🚀 Deployed to production by https://github.com/lakchote in version: 9.2.26-7 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

Add new ai review promt and prompt testing script

12d5806

melvin-bot bot assigned jmusial Oct 1, 2025

jmusial added 2 commits October 1, 2025 12:19

typo

1acc119

eof in gitignore

8abb58d

jmusial commented Oct 1, 2025

View reviewed changes

update shell lint

f6f437d

jmusial marked this pull request as ready for review October 1, 2025 11:50

jmusial requested a review from a team as a code owner October 1, 2025 11:50

melvin-bot bot requested review from brunovjk and removed request for a team October 1, 2025 11:50

adhorodyski reviewed Oct 1, 2025

View reviewed changes

jmusial added 2 commits October 1, 2025 15:15

remove prompt elements from testing script

21949da

encourage Claude to leave comments if uncertain

prevent from outputting useless comments

bfa84f2

war-in approved these changes Oct 1, 2025

View reviewed changes

adhorodyski reviewed Oct 3, 2025

View reviewed changes

update prompt formatting, remove testing script

9e15cda

adhorodyski approved these changes Oct 6, 2025

View reviewed changes

mountiny approved these changes Oct 6, 2025

View reviewed changes

mountiny merged commit c89cc25 into Expensify:main Oct 6, 2025
12 of 16 checks passed

OSBotify mentioned this pull request Oct 6, 2025

Deploy Checklist: New Expensify 2025-10-06 #71926

Closed

73 tasks


		Conditions: Flag ONLY when ALL of these are true:

		- Using .every(), .some(), .find(), .filter() or similar function

Conversation

jmusial commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Prompt testing

PR: Expensify card page uses navigation for magic code ✅

❌ [PERF-6]

PR: Delay emoji modal opening when keyboard is closing ✅

PR: Navigation initialization console errors fix ✅

PR Feat/update ai review prompt - test branch with violations ✅

❌ [PERF-4] Memoize objects and functions passed as props

❌ [PERF-2] Use early returns in array iteration methods

❌ [PERF-5] Use shallow comparisons instead of deep comparisons

❌ [PERF-6] Use specific properties as hook dependencies

❌ [PERF-1] No spread in list item's renderItem

PR: Add F1 Custom Avatars SVGs ✅

PR: WIP - Add card fraud alert ✅

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adhorodyski Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmusial commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melvin-bot bot commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmusial Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mountiny commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OSBotify commented Oct 6, 2025

Uh oh!

OSBotify commented Oct 6, 2025

Uh oh!

OSBotify commented Oct 7, 2025

Uh oh!

OSBotify commented Oct 8, 2025

jmusial commented Oct 1, 2025 •

edited

Loading

adhorodyski Oct 3, 2025 •

edited

Loading

jmusial commented Oct 1, 2025 •

edited

Loading

jmusial Oct 6, 2025 •

edited

Loading

mountiny commented Oct 6, 2025 •

edited

Loading