Skip to content
Closed
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
3 changes: 3 additions & 0 deletions .claude/skills/coding-standards/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ Coding standards for the Expensify App. Each standard is a standalone file in `r
- [CONSISTENCY-9](rules/consistency-9-file-naming.md) — Name files after what they export
- [CONSISTENCY-10](rules/consistency-10-jsdoc.md) — Follow the JSDoc style guidelines
- [CONSISTENCY-11](rules/consistency-11-no-todo-comments.md) — Track future work in an issue, not a TODO comment
- [CONSISTENCY-12](rules/consistency-12-callback-named-for-action.md) — Name callbacks for what they do, not the event they handle
- [CONSISTENCY-13](rules/consistency-13-document-props.md) — Document component props with a JSDoc block comment
- [CONSISTENCY-14](rules/consistency-14-new-file-header.md) — Non-trivial new files start with a header description

### Clean React Patterns
- [CLEAN-REACT-PATTERNS-0](rules/clean-react-0-compiler.md) — React Compiler compliance
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
---
ruleId: CONSISTENCY-12
title: Name callbacks for what they do, not the event they handle
---

## [CONSISTENCY-12] Name callbacks for what they do, not the event they handle

### Reasoning

Per `contributingGuides/STYLE.md`, a callback method should be named for the action it performs, not for the event slot it happens to be wired to. `toggleReport` tells the reader what happens; `onIconClick` only tells you where it was attached and has to be re-read to learn what it does. Naming by behavior keeps handlers reusable across triggers and searchable by intent. The `on*` / `handle*` naming belongs on the JSX prop, not on the function definition.

### Incorrect

```tsx
function ReportHeader() {
const onIconClick = () => Report.toggleReport(reportID);
const handleButtonPress = () => Navigation.dismissModal();

return <Icon onPress={onIconClick} />;
}
```

### Correct

```tsx
function ReportHeader() {
const toggleReport = () => Report.toggleReport(reportID);
const dismissModal = () => Navigation.dismissModal();

return <Icon onPress={toggleReport} />;
}
```

---

### Review Metadata

Flag ONLY when ALL of these are true:

- The changed code **declares** a function (function declaration or arrow/function assigned to a `const`/`let`) whose name matches `^(on|handle)[A-Z]` and describes the triggering event (e.g. `onIconClick`, `handleButtonPress`, `onRowClick`)
- The name refers to the event slot rather than the action performed, and a behavior-based name (`toggleReport`, `dismissModal`) would be clearer

**DO NOT flag if:**

- The `on*` / `handle*` name appears only as a JSX prop assignment or call site (e.g. `onPress={toggleReport}`) rather than as the function's own declaration
- The function genuinely represents a named event in an event system or implements an external API/interface that dictates the name (e.g. a required `onSuccess` callback prop, `onLayout`, DOM event handler contracts)
- The handler is a prop being passed through/forwarded rather than defined here
- The file is a test or story

**Search Patterns** (hints for reviewers):
- `const on[A-Z]` / `const handle[A-Z]` / `function on[A-Z]` / `function handle[A-Z]`
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
---
ruleId: CONSISTENCY-13
title: Document component props with a JSDoc block comment
---

## [CONSISTENCY-13] Document component props with a JSDoc block comment

### Reasoning

Per `contributingGuides/STYLE.md`, every component prop is documented with a `/** ... */` block comment above it so its purpose is clear at the definition site. This rule enforces the *presence* of that documentation when a new props type is added with no documented props at all. It is the companion to CONSISTENCY-10, which enforces JSDoc *style* and catches the mixed cases (a `//` comment on a prop, or an undocumented prop sitting next to documented siblings). Together they ensure every prop carries a `/** */` block.

### Incorrect

```tsx
type ButtonProps = {
isDisabled: boolean;
onPress: () => void;
};
```

### Correct

```tsx
type ButtonProps = {
/** Whether the button is disabled */
isDisabled: boolean;

/** Called when the button is pressed */
onPress: () => void;
};
```

---

### Review Metadata

Flag ONLY when ALL of these are true:

- The changed code adds (or newly introduces the members of) a component props type/interface - a `type`/`interface` whose name ends in `Props`
- It declares one or more of its own props
- **None** of those props has a `/** ... */` block comment above it

**DO NOT flag if:**

- At least one prop in the type is already documented with `/** */` (the mixed/undocumented-sibling and `//`-comment cases belong to CONSISTENCY-10, not here - avoid double-flagging)
- The type only re-exports, extends, intersects, or spreads props from a base type documented elsewhere and declares no new members of its own
- The props are inherited from a shared base type
- The file is a test or story

**Search Patterns** (hints for reviewers):
- Added `type ...Props = {` / `interface ...Props {` blocks whose members have no preceding `/** */`
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
---
ruleId: CONSISTENCY-14
title: Non-trivial new files start with a header description
---

## [CONSISTENCY-14] Non-trivial new files start with a header description

### Reasoning

Per the PR checklist, when a new file is added it should carry a short description of what it does and/or why it is needed at the top of the file, unless the code is self explanatory. A one-line header comment on a new library, hook, or util orients the next reader immediately instead of forcing them to reverse-engineer the module's purpose from its exports.

### Incorrect

A new `src/libs/CardUtils/deriveLimit.ts` that opens straight into code:

```tsx
import type {Card} from '@src/types/onyx';

function deriveLimit(card: Card): number {
// ...
}
```

### Correct

```tsx
/**
* Helpers for deriving a card's spend limit from its Onyx data.
*/
import type {Card} from '@src/types/onyx';

function deriveLimit(card: Card): number {
// ...
}
```

---

### Review Metadata

Flag ONLY when ALL of these are true:

- The change **adds a new file** (not a modification of an existing one) under `src/libs/**`, `src/hooks/**`, or a comparable non-trivial module directory
- The file contains non-trivial logic (more than a few lines; not a pure re-export or a single constant)
- Its first non-import, non-directive token is not a comment - there is no header comment describing the file

**DO NOT flag if:**

- The file is a barrel / `index.*` re-export file, a platform-suffixed variant (`.ios`/`.android`/`.native`/`.web`) of a file documented in its base variant, or a type-only declaration file
- The file is short and self explanatory (e.g. a tiny constant, a one-line util whose name says everything)
- The file is a test, story, snapshot, or generated/config file
- A header comment is already present

**Search Patterns** (hints for reviewers):
- Newly added files under `src/libs/**` / `src/hooks/**` whose first line is `import`/`export` with no leading `/** */` or `//` header
3 changes: 0 additions & 3 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,12 @@ This is a checklist for PR authors. Please make sure to complete all tasks and c
- [ ] MacOS: Chrome / Safari
- [ ] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
- [ ] I followed proper code patterns (see [Reviewing the code](https://github.com/Expensify/App/blob/main/contributingGuides/PR_REVIEW_GUIDELINES.md#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 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)
- [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- [ ] I followed the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/contributingGuides/PR_REVIEW_GUIDELINES.md)
- [ ] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like `Avatar`, I verified the components using `Avatar` are working as expected)
- [ ] 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](https://github.com/Expensify/App/blob/main/src/styles/utils/index.ts) function (i.e. `StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)`)
Expand Down
9 changes: 0 additions & 9 deletions contributingGuides/REVIEWER_CHECKLIST.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
- [ ] MacOS: Chrome / Safari
- [ ] 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 proper code patterns were followed (see [Reviewing the code](https://github.com/Expensify/App/blob/main/contributingGuides/PR_REVIEW_GUIDELINES.md#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 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)
Expand All @@ -27,16 +26,8 @@
- [ ] 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)
- [ ] 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](https://github.com/Expensify/App/blob/main/src/utils/index.ts) function (i.e. `StyleUtils.getBackgroundAndBorderStyle(theme.componentBG`)
Expand Down
Loading