Skip to content

[HOLD discussion] Add section for Proposing Performance Improvements #78757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Beamanator wants to merge 14 commits into
base: main
Choose a base branch
from
+93 −1
Open

[HOLD discussion] Add section for Proposing Performance Improvements #78757

Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9f1f69f
Add section for Proposing Performance Improvements
Beamanator Jan 1, 2026
2261661
move compensation out of main points
Beamanator Jan 1, 2026
b345460
Update contributingGuides/PERFORMANCE.md
Beamanator Jan 1, 2026
2035bae
Improve wording
Beamanator Jan 1, 2026
4f13d3c
Apply suggestions from code review
Beamanator Jan 1, 2026
5e6f307
Few updates
Beamanator Jan 1, 2026
1898c85
Merge branch 'beaman-addPerformanceProposalPlan' of github.com:Expens…
Beamanator Jan 1, 2026
24bdecc
Clarify which section is the tempalte
Beamanator Jan 1, 2026
54c6548
Merge branch 'main' of github.com:Expensify/App into beaman-addPerfor…
Beamanator Jan 5, 2026
fba17fc
Clarify point
Beamanator Jan 5, 2026
87c3dd5
Remove outdated instructions
Beamanator Jan 6, 2026
f71a522
Change GH tag to Slack tag
Beamanator Jan 6, 2026
51e8996
Add SO link for setting up slack keyword notification
Beamanator Jan 6, 2026
8d0fbd6
fix spelling
Beamanator Jan 6, 2026
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
94 changes: 93 additions & 1 deletion contributingGuides/PERFORMANCE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,4 +361,96 @@ Examples:
- [Remove shouldAdjustScrollView to avoid heavy rerender](https://github.com/Expensify/App/pull/66849) - removes hooks that were called only for Safari logic slowing down the `ReportScreen.tsx`
- [PopoverWithMeasuredContent optimization for mobile](https://github.com/Expensify/App/pull/68223) - returns early to avoid unnecessary calculations
- [Reduce confirm modal initial render count](https://github.com/Expensify/App/pull/67518) - returns early to reduce first load cost
- [Do not render PopoverMenu until it gets opened](https://github.com/Expensify/App/pull/67877) - adds a wrapper to control if `PopoverMenu` should be rendered
- [Do not render PopoverMenu until it gets opened](https://github.com/Expensify/App/pull/67877) - adds a wrapper to control if `PopoverMenu` should be rendered

# Proposing Performance Improvements

We are actively looking for contributions that improve the performance of the App, specifically regarding unnecessary re-renders, slow method executions, and user perceived latency.

If you haven't already, check out our [Contributing Guidelines](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md).

👉 **Before posting the proposal, please read through this whole process for important context and instructions.** Proposals that do not follow these guidelines cannot be accepted.

___

### Instructions for Submission
1. Copy the template below.
2. Fill out the details strictly following the guide.
3. Post it in `#expensify-open-source` with the title `[Performance Proposal] <Component_Name>`.

___

```
## 1. Component and Flow Description

**Component/Flow:** Describe the specific UI component or user flow being optimized.
- [Add details here]

**Preconditions:** List any specific setup required before reproducing the steps (e.g., "Workspace must have chat history").
- [Add details here]

**Reproduction Steps:** Provide a numbered list of steps to reproduce the performance issue (similar to a QA test case).
- [Add details here]

## 2. Required Tools
*I have verified these metrics using (check all that apply):*
- [ ] React DevTools Profiler
- [ ] Chrome Performance Tab
- [ ] JS Flame charts
- [ ] Hermes / Release Profiler traces
- [ ] Sentry (If you have access)

## 3. Before/After Metrics
*Please fill out the table below. If a metric is not applicable, write N/A.*

| Metric | Before | After | Improvement |
| :--- | :--- | :--- | :--- |
| **Render Count** | | | |
| **Execution Time** | | | |
| **Perceived Latency** | | | |

* **Device Used:** (e.g. iPhone 13, Pixel 6, Chrome on M1 Mac)
* **Evidence:** *(Attach screenshots of the profiler or logs for both Before and After below this section)*

## 4. Prerequisites & Eligibility
*To ensure proposals are measurable and based on realistic scenarios, you must meet the following criteria:*

- [ ] **Experience:** I have at least **1 merged PR** in the App repository.
Copy link
Contributor

@luacmartins luacmartins Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this check solves any problem, so I think we should remove this.

Suggested change
- [ ] **Experience:** I have at least **1 merged PR** in the App repository.

Copy link
Contributor

@mallenexpensify mallenexpensify Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's similar to how we require a new contributor to have a PR merged before working on multiple PRs. The problem for that was that new contributors would get buried with multiple issues and fall behind on them. I like that it keeps people with no knowledge or history of Expensify to bomb #expensify-open-source with a bunch of AI slop, hoping to get lucky with a proposal or two

Copy link
Contributor Author

@Beamanator Beamanator Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm leaning towards @luacmartins 's point that we don't necessarily need this restriction yet - it's a low bar so it probably won't stop many proposals, but we can easily add it in the future if we do find a decent amount of AI slop / proposals from people unfamiliar with our codebase & such

Copy link
Contributor

@mallenexpensify mallenexpensify Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👍 with start out without the requirement, easy to add later if we need/want. Let's keep 👀 peeled for spammers though so we can squash that practice early.

Copy link
Contributor Author

@Beamanator Beamanator Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100000% 👍 👍

Copy link
Contributor

@luacmartins luacmartins Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just block people who're abusing the system, but that's my two cents.

- [ ] **Test Environment:** I tested on a high-traffic account (instructions to create this [here](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts)).
- [ ] **Thresholds:** My proposal meets **at least one** of the following:
- [ ] > 20% reduction in Render Count
- [ ] > 20% reduction in Execution Time
- [ ] > 100ms reduction in Perceived Latency
Comment on lines +421 to +423
Copy link
Contributor

@luacmartins luacmartins Jan 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO reducing render count/execution should result in gains in perceived latency. The user doesn't care if the Render count went from 300 to 1, they care if the app is 300ms faster, so let's track just that one metric. I think this would help eliminate a bunch of proposals for Reduced render from X to Y that doesn't really change App performance

Copy link
Contributor Author

@Beamanator Beamanator Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luacmartins please feel free to bring up this point in the predesign discussion point here if you'd like to update this. We already got 7 👍 's for this point but we can always reconsider if you would like to provide your reasonings there

Copy link
Contributor

@luacmartins luacmartins Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised in Slack.


## 5. Pattern Detection & Prevention
*Can we prevent this from happening again?*
Copy link
Contributor

@tgolen tgolen Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "this" referring to? I don't quite understand the question.

Copy link
Contributor Author

@Beamanator Beamanator Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's referring to the previous state of "worse performance" - this is supposed to be generic so it can apply to all proposals, but maybe it's too unclear & therefore won't get any useful response 😅

Copy link
Contributor

@tgolen tgolen Jan 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think this is covered by one of the other points, but basically... I don't think we should assume that the performance problem being fix was bad, or an anti-pattern. A lot of times, it's just because it wasn't pre-optimized in the first place. So, maybe this could be something more like:

Is the code logic being optimized something that should be prevented from being added to the app in the future, for example, through an ESLint rule?

The answer might be "no", which is OK, but it should at least be considered.

Copy link
Contributor Author

@Beamanator Beamanator Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of times, it's just because it wasn't pre-optimized in the first place

That's def a good point 👍

How about we phrase it like this?

**Future Prevention:**
*Is the code logic being optimized something that should be prevented from being added to the app in the future (e.g., via an ESLint rule)?*
- [ ] Yes (Proposal: _________________)
- [ ] No (It's a valid pattern, just unoptimized here)

- [ ] **App-wide Audit:** I have checked for other places in the app that have this same performance problem and fixed them.
- [ ] **Shared Refactor:** This fixes a shared utility/component (e.g., `Avatar.ts`) used across the app.
- [ ] **Localized Fix:** This only affects this specific view.

## 6. Automated Tests & QA
*Tests are required by default. If you cannot add them, explain why.*
- [ ] **Unit Tests:** Added to prevent regression.
- [ ] **Reassure Tests:** Added (Required for execution time improvements).
- [ ] **Exception:** I cannot add automated tests because: _________________
- [ ] **Manual Verification:** I have included manual verification steps (Required).

## 7. Other Considerations & UX Risks
*Performance improvements should not change user experience and product design.*
- [ ] This change preserves existing UX (No visual/behavioral changes).
- [ ] This change alters UX (Description: _________________).
```

---

### Compensation
* **Bounty:** Accepted and merged performance improvements are eligible for a flat **$250 bounty**.
* **Scope:** We prefer smaller, atomic PRs. However, if multiple proposals are submitted for closely related logic that could have been one PR, we reserve the right to consolidate them.

___

### Review Process
1. **Peer Review:** Wait for **2 Expert Contributors** to approve your proposal.
2. **Internal Review:** Once approved by experts, comment `Proposal ready for final review - cc: perf-review` in slack to notify Internal Engineers that the proposal is ready a final review.
- Note: Internal Engineers can set up notifications for `perf-review` keyword as mentioned in [this Internal SO](https://stackoverflowteams.com/c/expensify/questions/23081/23082#23082).
3. **Approval:** **2 Internal Engineers** must approve before a GH issue is created.
Loading