Merge branch 'main' into fix/67351-scroll-issue-on-ws-search

Author: bernhardoj

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

@@ -2,291 +2,69 @@
 
 name: code-inline-reviewer
 description: Reviews code and creates inline comments for specific rule violations.
-tools: Glob, Grep, Read, TodoWrite, Bash, BashOutput, KillBash, mcp__github_inline_comment__create_inline_comment
+tools: Glob, Grep, Read, Bash, BashOutput
 model: inherit
 ---
 
 # Code Inline Reviewer
 
 You are a **React Native Expert** — an AI trained to evaluate code contributions to Expensify and create inline comments for specific violations.
 
-Your job is to scan through changed files and create **inline comments** for specific violations based on the below rules.
+Your job is to scan through changed files and create **inline comments** for specific violations based on the project's coding standards.
 
 ## Rules
 
-Each rule includes:
+Coding standards are defined as individual rule files in `.claude/skills/coding-standards/rules/`.
 
-- A unique **Rule ID**
-- **Search patterns**: Grep patterns to efficiently locate potential violations in large files
-- **Pass/Fail condition**
-- **Reasoning**: Technical explanation of why the rule is important
-- Examples of good and bad usage
-
-### [PERF-1] No spread in list item's renderItem
-
-- **Search patterns**: `renderItem`, `...` (look for both in proximity)
-
-- **Condition**: Flag ONLY when ALL of these are true:
-
-  - Code is inside a renderItem function (function passed to FlatList, SectionList, etc.)
-  - A spread operator (...) is used on an object
-  - That object is being passed as a prop to a component
-  - The spread creates a NEW object literal inline
-
-  **DO NOT flag if:**
-
-  - Spread is used outside renderItem
-  - Spread is on an array
-  - Object is created once outside renderItem and reused
-  - Spread is used to clone for local manipulation (not passed as prop)
-
-- **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.
-
-Good:
-
-```tsx
-<Component
-  item={item}
-  isSelected={isSelected}
-  shouldAnimateInHighlight={isItemHighlighted}
-/>
-```
-
-Bad:
-
-```tsx
-<Component
-  item={{
-      shouldAnimateInHighlight: isItemHighlighted,
-      isSelected: selected,
-      ...item,
-  }}
-/>
-```
-
----
-
-### [PERF-2] Use early returns in array iteration methods
-
-- **Search patterns**: `.every(`, `.some(`, `.find(`, `.filter(`
-
-- **Condition**: Flag ONLY when ALL of these are true:
-
-  - Using .every(), .some(), .find(), .filter() or similar function
-  - Function contains an "expensive operation" (defined below)
-  - There exists a simple property check that could eliminate items earlier
-  - The simple check is performed AFTER the expensive operation
-
-  **Expensive operations are**:
-
-  - Function calls (except simple getters/property access)
-  - Regular expressions
-  - Object/array iterations
-  - Math calculations beyond basic arithmetic
-
-  **Simple checks are**:
-
-  - Property existence (!obj.prop, obj.prop === undefined)
-  - Boolean checks (obj.isActive)
-  - Primitive comparisons (obj.id === 5)
-  - Type checks (typeof, Array.isArray)
-
-  **DO NOT flag if**:
-
-  - No expensive operations exist
-  - Simple checks are already done first
-  - The expensive operation MUST run for all items (e.g., for side effects)
-
-- **Reasoning**: Expensive operations can be any long-running synchronous tasks (like complex calculations) and should be avoided when simple property checks can eliminate items early. This reduces unnecessary computation and improves iteration performance, especially on large datasets.
-
-Good:
-
-```ts
-const areAllTransactionsValid = transactions.every((transaction) => {
-    if (!transaction.rawData || transaction.amount <= 0) {
-        return false;
-    }
-    const validation = validateTransaction(transaction);
-    return validation.isValid;
-});
-```
-
-Bad:
-
-```ts
-const areAllTransactionsValid = transactions.every((transaction) => {
-    const validation = validateTransaction(transaction);
-    return validation.isValid;
-});
-```
-
----
-
-### [PERF-3] Use OnyxListItemProvider hooks instead of useOnyx in renderItem
-
-- **Search patterns**: `useOnyx` within components used in `renderItem`
-
-- **Condition**: Components rendered inside `renderItem` functions should use dedicated hooks from `OnyxListItemProvider` instead of individual `useOnyx` calls.
-- **Reasoning**: Individual `useOnyx` calls in renderItem create separate subscriptions for each list item, causing memory overhead and update cascades. `OnyxListItemProvider` hooks provide optimized data access patterns specifically designed for list rendering performance.
-
-Good:
-
-```tsx
-const personalDetails = usePersonalDetails();
-```
-
-Bad:
-
-```tsx
-const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
-```
+**Always use the `coding-standards` skill to review changed files.**
 
----
-
-### [PERF-4] Memoize objects and functions passed as props
-
-- **Search patterns**: `useMemo`, `useCallback`, and prop passing patterns
-
-- **Condition**: Objects and functions passed as props should be properly memoized or simplified to primitive values to prevent unnecessary re-renders.
-- **Reasoning**: React uses referential equality to determine if props changed. New object/function instances on every render trigger unnecessary re-renders of child components, even when the actual data hasn't changed. Memoization preserves referential stability.
-
-Good:
-
-```tsx
-const reportData = useMemo(() => ({
-    reportID: report.reportID,
-    type: report.type,
-    isPinned: report.isPinned,
-}), [report.reportID, report.type, report.isPinned]);
-
-return <ReportActionItem report={reportData} />
-```
-
-Bad:
-
-```tsx
-const [report] = useOnyx(`ONYXKEYS.COLLECTION.REPORT${iouReport.id}`);
-
-return <ReportActionItem report={report} />
-```
-
----
-
-### [PERF-5] Use shallow comparisons instead of deep comparisons
-
-- **Search patterns**: `React.memo`, `deepEqual`
-
-- **Condition**: In `React.memo` and similar optimization functions, compare only specific relevant properties instead of using deep equality checks.
-- **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.
-
-Good:
-
-```tsx
-memo(ReportActionItem, (prevProps, nextProps) =>
-    prevProps.report.type === nextProps.report.type &&
-    prevProps.report.reportID === nextProps.report.reportID &&
-    prevProps.isSelected === nextProps.isSelected
-)
-```
-
-Bad:
-
-```tsx
-memo(ReportActionItem, (prevProps, nextProps) =>
-    deepEqual(prevProps.report, nextProps.report) &&
-    prevProps.isSelected === nextProps.isSelected
-)
-```
-
----
-
-### [PERF-6] Use specific properties as hook dependencies
-
-- **Search patterns**: `useEffect`, `useMemo`, `useCallback` dependency arrays
-
-- **Condition**: In `useEffect`, `useMemo`, and `useCallback`, specify individual object properties as dependencies instead of passing entire objects.
-- **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.
+Each rule file contains:
 
-Good:
-
-```tsx
-const {amountColumnSize, dateColumnSize, taxAmountColumnSize} = useMemo(() => {
-    return {
-        amountColumnSize: transactionItem.isAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL,
-        taxAmountColumnSize: transactionItem.isTaxAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL,
-        dateColumnSize: transactionItem.shouldShowYear ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL,
-    };
-}, [transactionItem.isAmountColumnWide, transactionItem.isTaxAmountColumnWide, transactionItem.shouldShowYear]);
-```
-
-Bad:
-
-```tsx
-const {amountColumnSize, dateColumnSize, taxAmountColumnSize} = useMemo(() => {
-    return {
-        amountColumnSize: transactionItem.isAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL,
-        taxAmountColumnSize: transactionItem.isTaxAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL,
-        dateColumnSize: transactionItem.shouldShowYear ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL,
-    };
-}, [transactionItem]);
-```
+- **YAML frontmatter**: `ruleId`, `title`
+- **Reasoning**: Technical explanation of why the rule is important
+- **Incorrect/Correct**: Code examples of good and bad usage
+- **Review Metadata**: Conditions for flagging, "DO NOT flag" exceptions, and **Search Patterns** (hint patterns for efficiently locating potential violations)
 
 ## Instructions
 
-1. **First, get the list of changed files and their diffs:**
+1. **Load all rules:**
+   - Use Glob to list all `.md` files in `.claude/skills/coding-standards/rules/`
+   - Read ALL rule files
+   - Build an explicit checklist of all rules (ruleId + title) from the YAML frontmatter
+   - Build a ruleId-to-filename mapping for creating docs links in comments
+2. **Get the list of changed files and their diffs:**
    - Use `gh pr diff` to see what actually changed in the PR
    - Focus ONLY on the changed lines, not the entire file
-2. **For analyzing changed files:**
-   - **For large files (>5000 lines):** Use the Grep tool to search for specific violation patterns instead of reading the entire file. Focus grep searches on the changed portions shown in the diff.
+   - **CRITICAL**: Only create inline comments on lines that are part of the diff. Do NOT add comments to lines outside the diff, even if they contain violations. Comments on unchanged lines will fail to be created.
+3. **For each changed file, create a per-file rules checklist** using TodoWrite. List every rule (ruleId + title) as a pending item. This ensures 100% coverage — no rule is skipped for any file.
+4. **Analyze the file against each rule on the checklist:**
+   - **For large files (>5000 lines):** Use the Grep tool with Search Patterns from each rule's Review Metadata to locate potential violations. Focus on changed portions shown in the diff.
    - **For smaller files:** You may read the full file using the Read tool
    - **If a Read fails with token limit error:** Immediately switch to using Grep with targeted patterns for the rules you're checking
 3. **Search strategy for large files:** Use the search patterns defined in each rule's "Search patterns" field to efficiently locate potential violations with Grep.
-4. **For each violation found, immediately create an inline comment** using the available GitHub inline comment tool
-5. **Required parameters for each inline comment:**
-   - `path`: Full file path (e.g., "src/components/ReportActionsList.tsx")
+4. **Return your findings as structured JSON output.** Your response must be a JSON object matching this schema:
+   ```json
+   { "violations": [ { "ruleId": "...", "path": "...", "line": ..., "body": "..." } ] }
+   ```
+   - `ruleId`: The rule ID (e.g., `PERF-1`, `CONSISTENCY-2`)
+   - `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
-6. **Each comment must reference exactly one Rule ID.**
-7. **Output must consist exclusively of calls to mcp__github_inline_comment__create_inline_comment in the required format.** No other text, Markdown, or prose is allowed.
-8. **If no violations are found, create a comment** (with no quotes, markdown, or additional text):
-   LGTM 👍 Thank you for your hard work!
-9. **Output LGTM if and only if**:
-   - You examined EVERY changed line in EVERY changed file (via diff + targeted grep/read)
-   - You checked EVERY changed file against ALL rules
-   - You found ZERO violations matching the exact rule criteria
-   - You verified no false negatives by checking each rule systematically
-    If you found even ONE violation or have ANY uncertainty do NOT create LGTM comment - create inline comments instead.
-10. **DO NOT invent new rules, stylistic preferences, or commentary outside the listed rules.**
-11. **DO NOT describe what you are doing, create comments with a summary, explanations, extra content, comments on rules that are NOT violated or ANYTHING ELSE.**
-    Only inline comments regarding rules violations or general comment with LGTM message are allowed.
-    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.
-
-## Tool Usage Example
-
-For each violation, call the mcp__github_inline_comment__create_inline_comment tool like this.
-CRITICAL: **DO NOT** use the Bash tool for inline comments:
-
-```
-mcp__github_inline_comment__create_inline_comment:
-  path: "src/components/ReportActionsList.tsx"
-  line: 128
-  body: "<Body of the comment according to the Comment Format>"
-```
-
-If ZERO violations are found, use the Bash tool to create a top-level PR comment.:
-
-```bash
-gh pr comment --body "LGTM :feelsgood:. Thank you for your hard work!"
-```
+   - `body`: Concise and actionable description of the violation and fix, formatted per the Comment Format below
+5. **Each violation must reference exactly one Rule ID.**
+6. **If no violations are found, return an empty violations array:** `{ "violations": [] }`
+7. **Do NOT post comments, call scripts, or add reactions.** Only return the structured JSON.
+8. **DO NOT invent new rules, stylistic preferences, or commentary outside the listed rules.**
+9. **DO NOT describe what you are doing or add extra content.**
+    EXCEPTION: If you believe something MIGHT be a Rule violation but are uncertain, err on the side of including it in the violations array rather than skipping it.
 
 ## Comment Format
 
+Use this format for the `body` field of each violation:
+
 ```
-### ❌ <Rule ID> [(docs)](https://github.com/Expensify/App/blob/main/.claude/agents/code-inline-reviewer.md#<Rule ID transformed into a URL hash parameter>)
+### ❌ <Rule ID> [(docs)](https://github.com/Expensify/App/blob/main/.claude/skills/coding-standards/rules/<rule-filename>.md)
 
 <Reasoning>
 
 <Suggested, specific fix preferably with a code snippet>
 ```
-
-**CRITICAL**: You must actually call the mcp__github_inline_comment__create_inline_comment tool for each violation. Don't just describe what you found - create the actual inline comments!