Merge branch 'Expensify:main' into openPolicyCompanyCardsFeed-emailList

Author: fedirjh

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

@@ -11,6 +11,8 @@ You are **Support Doc Optimizer** — an AI trained to evaluate HelpDot articles
 
 Your job is to scan through changed documentation files and create **inline comments** for specific violations based on the three core criteria below.
 
+**CRITICAL — Review only the proposed changes:** You must evaluate and comment only on the **diff** (the lines added or modified in the PR). Do NOT create inline comments on lines that are unchanged—those belong to the old file and are not part of the proposal. Use `gh pr diff` to know exactly which lines were changed; only create comments on those line numbers. Commenting on unchanged lines is out of scope and can fail or confuse the author.
+
 ## 1. Readability Violations (Create inline comments for)
 - Poor sentence clarity, grammar, or scannability issues
 - Illogical flow or ordering of sections  
@@ -20,6 +22,7 @@ Your job is to scan through changed documentation files and create **inline comm
 
 ## 2. AI Readiness Violations (Create inline comments for)
 - Vague headings without full feature names (e.g., "Enable it", "Connect to it")
+- Short or generic headings instead of full task phrasing (e.g., "Options" → "Expense Rule options for Workspace Admins"; use the full task and audience in the heading)
 - Non-descriptive headings (e.g., "Where to find it" vs "Where to find Statement Matching")  
 - Vague references like "this," "that," or "it" without clear context
 - Missing or incomplete YAML metadata:
@@ -28,11 +31,14 @@ Your job is to scan through changed documentation files and create **inline comm
 title: [Full article title]
 description: [Concise, benefit-focused summary] 
 keywords: [feature name, related terms, navigation path, etc.]
+internalScope: Audience is [target role]. Covers [main topic]. Does not cover [excluded areas].
 ---
 ```
-- Missing breadcrumb paths below H1 (Settings > Workspaces > People)
+  - **internalScope** must always be present. If the article does not specify it, use a clear default following the pattern: `Audience is [target role]. Covers [main topic]. Does not cover [excluded areas].`
 - Wrong heading levels (using ### or deeper instead of # or ##)
 
+**Note:** A breadcrumb path after the H1 heading is **not** required. Do not flag missing breadcrumbs as a violation.
+
 ## 3. Expensify Style Compliance Violations (Create inline comments for)
 - Voice and tone issues:
   - Not casual yet professional
@@ -52,11 +58,13 @@ keywords: [feature name, related terms, navigation path, etc.]
 
 ## Instructions
 
-1. **First, get the list of changed files:**
-   - Use `gh pr diff` to see what actually changed in the PR
-   - Focus ONLY on documentation files (*.md, *.csv, etc.)
+1. **Get the diff and scope (required):**
+   - Use `gh pr diff` to see the exact lines added or modified in the PR
+   - Identify which file paths and line numbers are in the diff—these are the **only** lines you may comment on
+   - Focus only on documentation files (*.md, *.csv, etc.)
 
 2. **For analyzing changed files:**
+   - **Restrict analysis to the diff:** When checking for violations, evaluate only content that appears on added or modified lines. If you read a full file for context, do not create inline comments on line numbers that are not part of the diff.
    - **Use a hybrid approach** because different violations require different analysis methods:
      - **Grep is suitable for pattern-based violations only:**
        - Terminology violations ("policy" → "workspace", "user" → "member")
@@ -75,7 +83,7 @@ keywords: [feature name, related terms, navigation path, etc.]
 
 4. **Required parameters for each inline comment:**
    - `path`: Full file path (e.g., "docs/articles/new-expensify/chat/Create-a-New-Chat.md")
-   - `line`: Line number where the issue occurs
+   - `line`: Line number where the issue occurs — **must be a line that appears in the PR diff (added or modified)**. Do not use line numbers from unchanged portions of the file.
    - `body`: Concise description of the violation and fix
 
 ## Tool Usage Example

.claude/agents/helpdot-summary-reviewer.md

@@ -11,6 +11,8 @@ You are a documentation quality specialist that provides comprehensive assessmen
 
 Your job is to analyze all changed files and provide a single, comprehensive summary review with scores and overall recommendations.
 
+**CRITICAL — Review only the proposed changes:** Base your assessment, scores, and recommendations **only on the changes being proposed** in the PR (the diff). Use `gh pr diff` to see what was added or modified. Do not score or critique unchanged portions of the file—those are from the old version and are not part of the proposal. Evaluate and feedback only on the diff.
+
 ## Scoring Criteria
 
 ### 1. Readability (1-10)
@@ -21,12 +23,13 @@ Your job is to analyze all changed files and provide a single, comprehensive sum
 - Proper use of formatting elements
 
 ### 2. AI Readiness (1-10) 
-- Descriptive headings with full feature names
+- Descriptive headings with full feature names and full task phrasing (e.g., "Expense Rule options for Workspace Admins" not "Options")
 - Clear context without vague references
-- Proper YAML metadata structure
-- Breadcrumb navigation paths
+- Proper YAML metadata structure, including **internalScope** in the form: `Audience is [target role]. Covers [main topic]. Does not cover [excluded areas].` (use a clear default if not provided)
 - Consistent heading hierarchy (# and ## only)
 
+**Note:** Breadcrumb paths after H1 are not required; do not penalize for their absence.
+
 ### 3. Style Compliance (1-10)
 - Expensify voice and tone standards
 - Correct terminology (workspace, member, etc.)
@@ -64,9 +67,10 @@ Provide your assessment as a **top-level PR comment** using this format:
 
 ## Instructions
 
-1. **Analyze all changed documentation files**
-2. **Look for patterns and overall quality trends**
-3. **Provide balanced feedback** (both positive and areas for improvement)
-4. **Focus on the big picture** rather than individual line issues
-5. **Use Bash(gh pr comment:*) tool** to post the summary comment
-6. **Reference that inline comments provide specific details**
+1. **Get the diff first:** Use `gh pr diff` to see the exact proposed changes. Your entire assessment (scores, findings, recommendations) must be based only on added or modified lines—not on unchanged content from the old file.
+2. **Analyze only the proposed changes** in each documentation file
+3. **Look for patterns and overall quality trends** within the diff
+4. **Provide balanced feedback** (both positive and areas for improvement) — only for the proposed changes
+5. **Focus on the big picture** rather than individual line issues
+6. **Use Bash(gh pr comment:*) tool** to post the summary comment
+7. **Reference that inline comments provide specific details**

.claude/skills/coding-standards/rules/perf-11-optimize-data-selection.md

@@ -7,51 +7,145 @@ title: Optimize data selection and handling
 
 ### Reasoning
 
-Using broad data structures or performing unnecessary data operations causes excessive re-renders and degrades performance. Selecting specific fields and avoiding redundant operations reduces render cycles and improves efficiency.
+`useOnyx` supports an optional `selector` to narrow data before it reaches the component. Selectors control both **what** data the component receives and **how** Onyx detects changes:
+
+- **With a selector**: Onyx runs `deepEqual` on the selector output to decide whether to re-render. This guards against unnecessary re-renders when unrelated data changes, but the comparison cost scales with the size of the output.
+- **Without a selector**: Onyx uses `shallowEqual` on raw references, which is much cheaper. However, the component will re-render whenever **any** part of the subscribed data changes, since there is no narrowing to filter out irrelevant updates.
+
+This means selectors are a double-edged sword. A well-written selector that returns a primitive or a small object is highly effective — it skips re-renders when unrelated data changes, and `deepEqual` on a small value is trivial. But a poorly-written selector that returns a large object, a full collection, or a non-plain type like `Set`/`Map` makes things worse — it forces an expensive `deepEqual` on every Onyx update with no re-render savings.
+
+**When to use a selector:**
+- To pick a few fields from a single Onyx key (reduces re-renders from unrelated field changes)
+- To compute a final scalar (boolean, number, string) from a larger dataset
+
+**When NOT to use a selector:**
+- To transform or reshape data without reducing its size — subscribe without a selector and transform inline instead
+- To extract a large sub-property (e.g., `(data) => data?.reports`) — just access it directly after the hook
+- To filter/map entire collections into arrays — the output is still large, `deepEqual` still expensive
+- To return `Set` or `Map` — `deepEqual` is extremely slow on these types
 
 ### Incorrect
 
 ```tsx
-function UserProfile({ userId }) {
-  const [user] = useOnyx(`${ONYXKEYS.USER}${userId}`);
-  // Component re-renders when any user field changes, even unused ones
-  return <Text>{user?.name}</Text>;
+// BAD: No selector — component re-renders when any user field changes, even unused ones
+function UserProfile({userId}) {
+    const [user] = useOnyx(`${ONYXKEYS.USER}${userId}`);
+    return <Text>{user?.name}</Text>;
 }
+
+// BAD: Selector maps an entire collection — deepEqual must compare every item
+const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {
+    selector: (policies) =>
+        Object.fromEntries(
+            Object.entries(policies ?? {}).map(([key, policy]) => [
+                key,
+                {id: policy?.id, name: policy?.name, type: policy?.type},
+            ]),
+        ),
+});
+
+// BAD: Selector extracts a large sub-property without narrowing
+const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {
+    selector: (data) => data?.reports,
+});
+
+// BAD: Selector filters/maps a collection into an array — deepEqual on every item
+const [archivedReportIdsArray] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS, {
+    selector: (data): string[] =>
+        Object.entries(data ?? {})
+            .filter(([, value]) => value?.isArchived)
+            .map(([key]) => key),
+});
+
+// BAD: Selector returns a Set — deepEqual is extremely slow on Sets
+const [archivedReportIds] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS, {
+    selector: (data): Set<string> => {
+        const ids = new Set<string>();
+        Object.entries(data ?? {}).forEach(([key, value]) => {
+            if (value?.isArchived) {
+                ids.add(key);
+            }
+        });
+        return ids;
+    },
+});
+
+// BAD: Selector returns an array when the component only needs a boolean
+const [reportSummaries] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
+    selector: (reports) =>
+        Object.values(reports ?? {}).filter((r) => r?.total === 0),
+});
+const hasEmptyReports = reportSummaries.length > 0;
 ```
 
 ### Correct
 
 ```tsx
-function UserProfile({ userId }) {
-  const [user] = useOnyx(`${ONYXKEYS.USER}${userId}`, {
-    selector: (user) => ({
-      name: user?.name,
-      avatar: user?.avatar,
-    }),
-  });
-  return <Text>{user?.name}</Text>;
+// GOOD: Selector picks only the fields the component needs from a single item
+function UserProfile({userId}) {
+    const [user] = useOnyx(`${ONYXKEYS.USER}${userId}`, {
+        selector: (user) => ({
+            name: user?.name,
+            avatar: user?.avatar,
+        }),
+    });
+    return <Text>{user?.name}</Text>;
 }
+
+// GOOD: No selector on collection — shallowEqual is cheap, transform inline
+const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);
+const mappedPolicies = Object.fromEntries(
+    Object.entries(policies ?? {}).map(([key, policy]) => [
+        key,
+        {id: policy?.id, name: policy?.name, type: policy?.type},
+    ]),
+);
+
+// GOOD: No selector — access the property directly
+const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES);
+const reports = reportAttributes?.reports;
+
+// GOOD: No selector — filter and compute Set inline
+const [reportNameValuePairs] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS);
+const archivedReportIds = new Set(
+    Object.entries(reportNameValuePairs ?? {})
+        .filter(([, value]) => value?.isArchived)
+        .map(([key]) => key),
+);
+
+// GOOD: Selector computes the final boolean directly
+const [hasEmptyReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
+    selector: (reports) =>
+        Object.values(reports ?? {}).some((r) => r?.total === 0),
+});
 ```
 
 ---
 
 ### Review Metadata
 
-Flag ONLY when ALL of these are true:
+Flag ONLY when ANY of these are true:
 
-- A component uses a broad data structure (e.g., entire object) without selecting specific fields
-- This causes unnecessary re-renders when unrelated fields change
-- OR unnecessary data filtering/fetching is performed (excluding necessary data, fetching already available data)
+- A component uses `useOnyx` and either:
+  - Subscribes to a broad data structure without selecting specific fields, causing re-renders when unrelated fields change
+  - Uses a `selector` whose output is still large or complex (e.g., full collection, large mapped/transformed result, `Set`, `Map`), or returns an intermediate data structure that is further reduced by the component
+- A selector on a collection references other large external datasets (e.g., another Onyx collection passed via closure) and iterates over them on every change to the subscribed collection, compounding the computation cost on unrelated updates
 
 **DO NOT flag if:**
 
-- Specific fields are already being selected or the data structure is static
-- The filtering is necessary for correct functionality
-- The fetched data is required and cannot be derived from existing data
-- The function requires the entire object for valid operations
+- The selector returns a primitive value (`boolean`, `string`, `number`, `undefined`)
+- The selector returns a small object with only a few fields picked from a single item (not a collection)
+- The selector meaningfully reduces a large dataset to a small result (e.g., a primitive or a few items) by iterating over the subscribed collection itself — the `deepEqual` cost on a small result is negligible
+- The `useOnyx` call is on a single-item key (not a collection), and the selector picks specific fields
+- The data structure is static or the function requires the entire object for valid operations
 
 **Search Patterns** (hints for reviewers):
 - `useOnyx`
 - `selector`
-- `\.filter\(`
-- `\.map\(`
+- `useOnyx.*selector`
+- `selector.*=>`
+- `new Set\(`
+- `new Map\(`
+- `Object\.fromEntries`
+- `Object\.entries.*\.map`
+- `Object\.values.*\.filter`

.claude/skills/coding-standards/rules/ui-1-correct-loading-indicator.md

@@ -0,0 +1,96 @@
+---
+ruleId: UI-1
+title: Use the correct loading indicator based on navigation context
+---
+
+## [UI-1] Use the correct loading indicator based on navigation context
+
+### Reasoning
+
+If loading hangs, users need an escape route. When navigation (back button, close) is visible alongside the loader, users can escape - use `ActivityIndicator`. When no navigation is visible, users are trapped - use `FullscreenLoadingIndicator` with `shouldUseGoBackButton={true}` which shows an emergency "Go Back" button after timeout. This prop is being migrated to become default, so set it explicitly for now.
+
+### Incorrect
+
+```tsx
+// Header and FullscreenLoadingIndicator in SAME return - use ActivityIndicator
+<ScreenWrapper>
+    <HeaderWithBackButton title="Settings" />
+    <FullscreenLoadingIndicator />
+</ScreenWrapper>
+
+// No navigation, missing shouldUseGoBackButton - user trapped if loading hangs
+function ValidateLoginPage() {
+    return <FullscreenLoadingIndicator />;
+}
+
+// ActivityIndicator as sole content without navigation - use FullscreenLoadingIndicator
+function AuthLoadingPage() {
+    return (
+        <View style={[styles.flex1, styles.justifyContentCenter, styles.alignItemsCenter]}>
+            <ActivityIndicator size="large" />
+        </View>
+    );
+}
+```
+
+### Correct
+
+```tsx
+// No navigation in return - FullscreenLoadingIndicator with shouldUseGoBackButton
+function ValidateLoginPage() {
+    return <FullscreenLoadingIndicator shouldUseGoBackButton />;
+}
+
+// Loader and navigation in DIFFERENT returns - OK to use FullscreenLoadingIndicator
+function SettingsPage() {
+    if (isLoading) {
+        return <FullscreenLoadingIndicator shouldUseGoBackButton />;
+    }
+    return (
+        <ScreenWrapper>
+            <HeaderWithBackButton title="Settings" />
+            <Content />
+        </ScreenWrapper>
+    );
+}
+
+// Header visible during loading - use ActivityIndicator
+function SettingsPage() {
+    return (
+        <ScreenWrapper>
+            <HeaderWithBackButton title="Settings" />
+            {isLoading ? (
+                <View style={[styles.flex1, styles.justifyContentCenter, styles.alignItemsCenter]}>
+                    <ActivityIndicator size="large" />
+                </View>
+            ) : (
+                <Content />
+            )}
+        </ScreenWrapper>
+    );
+}
+```
+
+---
+
+### Review Metadata
+
+Flag ONLY when ANY of these patterns is found:
+
+- `FullscreenLoadingIndicator` and `HeaderWithBackButton` (or other navigation like close button) are **both under the same JSX tree** (not separated by conditionals)
+- `FullscreenLoadingIndicator` without `shouldUseGoBackButton` prop when **no navigation component** (e.g., `HeaderWithBackButton`, close button) **is rendered in the same return statement or conditional branch**
+- `ActivityIndicator` as the **sole/main screen content** (flex:1 container, early return) without any navigation component (e.g., `HeaderWithBackButton`, close button) **in the same return statement or conditional branch**
+
+**DO NOT flag if:**
+
+**For `FullscreenLoadingIndicator`:**
+- Visibility is controlled by `FullScreenLoaderContext`
+- Navigation visible in different conditional branches (separate return statement) AND has `shouldUseGoBackButton={true}`
+
+**For `ActivityIndicator`:**
+- Used within interactive UI elements (buttons, list items, cards) where user can still interact with surrounding navigation
+
+**Search Patterns** (hints for reviewers):
+- `FullscreenLoadingIndicator`
+- `FullScreenLoadingIndicator`
+- `ActivityIndicator`

.claude/skills/playwright-app-testing/SKILL.md

@@ -30,12 +30,10 @@ ps aux | grep "webpack" | grep -v grep
 ## Playwright Testing Workflow
 
 1. **Verify server**: Check webpack process is running
-2. **Navigate**: Use `mcp__playwright__browser_navigate` to `https://dev.new.expensify.com:8082/`
-3. **Interact**: Use Playwright MCP tools including:
-   - **Inspection**: `browser_snapshot`, `browser_take_screenshot`, `browser_console_messages`
-   - **Interaction**: `browser_click`, `browser_type`, `browser_fill_form`, `browser_hover`
-   - **Navigation**: `browser_navigate_back`, `browser_tabs`, `browser_wait_for`
-   - All other Playwright tools as needed
+2. **Navigate**: Open `https://dev.new.expensify.com:8082/` in the browser
+3. **Interact**: Use Playwright MCP tools to inspect, click, type, and navigate
+
+Do NOT add arbitrary waits after actions. Instead, take a snapshot to check the result and only add short waits if the page hasn't updated yet.
 
 ## Dev Environment Sign-In