[Due for payment 2025-06-17] [$500] package.json scripts: lint-changed runs on extraneous files

[MacroLens]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: NA
Reproducible in staging?: NA
Reproducible in production?: NA
If this was caught during regression testing, add the test name, ID and link from TestRail: NA
Email or phone of affected tester (no customers): NA
Logs: NA
Expensify/Expensify Issue URL: NA
Issue reported by:
Slack conversation (hyperlinked to channel name): NA

Action Performed:

Break down in numbered steps

1. Checkout Expensify/App main at 02e9906
2. Create a branch git checkout -b test_lint_changed
3. Setup Node, nvm install && npm install
4. Run npm run lint-changed

Expected Result:

No files were changed on the branch test_lint_changed so no lint errors should return.

Actual Result:

The lint-changed script runs over all files changed on main since 02e9906 upto the HEAD.

This leads to slow lint checks and is hard to manage when working on a pull request.

Workaround:

Before running the lint-changed script, merge the MAIN branch into the development branch (test_lint_changed). git merge main

It is difficult to work on pull requests when you need to merge so frequently to prevent this from happening.

Platforms:

Select the officially supported platforms where the issue was reproduced:

• Android: App
• Android: mWeb Chrome
• iOS: App
• iOS: mWeb Safari
• iOS: mWeb Chrome
• Windows: Chrome
• MacOS: Chrome / Safari
• MacOS: Desktop

Platforms Tested:

On which of our officially supported platforms was this issue tested:

• Android: App
• Android: mWeb Chrome
• iOS: App
• iOS: mWeb Safari
• iOS: mWeb Chrome
• Windows: Chrome
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @sonialiap / @sonialiap

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021927626789806626747
• Upwork Job ID: 1927626789806626747
• Last Price Increase: 2025-06-11

[melvin-bot]

📣 @MacroLens! 📣
Please report bugs or suggest features in the #expensify-open-source Slack channel, don't directly open issues in this repo!
Instructions here to join the channel 📖

[melvin-bot]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[MacroLens]

I haven't been added to the slack channel, so I posted this here to give visibility to the issue.

[MacroLens]

Proposal

Please re-state the problem that we are trying to solve in this issue.

npm run lint-changed lints extraneous files leading to long lint workflow
runtimes and lint errors for unchanged files.

What is the root cause of that problem?

In the package.json
is the lint-changed script

App/package.json

Line 49 in d17b700

"lint-changed": "NODE_OPTIONS=--max_old_space_size=8192 eslint --max-warnings=0 --config ./.eslintrc.changed.js $(git diff --diff-filter=AM --name-only origin/main HEAD -- \"*.ts\" \"*.tsx\")",

The inner git diff that runs computes what files that the lint will be performed on.
$(git diff --diff-filter=AM --name-only origin/main HEAD -- "*.ts" "*.tsx")

This computes the typescript files that were added or modified that differ on the HEAD of the main
branch and current branch (we call dev).

This is not a problem when the main branch is behind the dev branch, i.e.
at the point of branch main does not have any new commits added to it.
But as other branches get merged into main the HEAD of main moves forward,
so this diff will return the files that have been changed on the dev branch
and on the main branch since the branching point. At the time of writing I saw
298 different files.

git diff --diff-filter=AM --name-only origin/main HEAD -- "*.ts" "*.tsx" | wc -l

So the lint-changed script will run on multiple files that were not changed
on the dev branch.

This affects the workflow:

App/.github/workflows/lint-changed.yml

Lines 29 to 35 in d17b700

	- name: Run ESLint to check for deprecation warnings
	run: |
	# This will just fetch the latest commit from main
	git fetch origin main --no-tags --depth=1
	# shellcheck disable=SC2046
	npm run lint-changed

Running the steps for reproduction on my local resulting in a lint job taking:

real    2m43.779s
user    4m0.110s
sys     0m10.731s

and 65 lint errors (at time of writing, may change as HEAD moves in main).

If you apply the workaround by merging to main first:

real    0m0.999s
user    0m1.217s
sys     0m0.209s

What changes do you think we should make in order to solve the problem?

Rather than running:

git diff --diff-filter=AM --name-only origin/main HEAD -- "*.ts" "*.tsx"

in the package.json.
We should be running:

git diff --diff-filter=AM --name-only $(git merge-base origin/main HEAD) HEAD -- "*.ts" "*.tsx"

This finds the common ancestor of origin/main and our current development branch.
So only changes that were made on the development branch will be considered when
performing the lint-changed script.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

Because this issue affects the workflow.
You could create a merge commit with origin/main like:

      - name: Run ESLint to check for deprecation warnings
        run: |
          # This will just fetch the latest commit from main
          git fetch origin main --no-tags --depth=1
          # Merge and commit a dummy commit.
          git merge origin/main --no-commit
          git commit -m "github actions: workflow merge main to run lint."
          # shellcheck disable=SC2046
          npm run lint-changed

This may address the issue of long lint-jobs, but only for the GitHub Action and wouldn't
affect local development.

This bug was introduced in #49102
with its corresponding PR #49165 (review).

The intention was given in the issue:

Rather than undertaking a coordinated migration of all our withOnyx usages to its successor useOnyx, let's instead introduce a new PR check in our CI such that if you modify a file that uses withOnyx, the CI will fail on your PR, regardless of whether you introduced that usage. This way, we'll force an incremental migration as files are changed without putting in a large upfront investment. This low-urgency incremental migration matches the urgency of the problem.

The intention was only on files you changed in the PR, not all files that have
been changed on main since the PR was last merged with main.

[MacroLens]

@sonialiap Apologies for opening an issue directly on GitHub, but I don't have access to slack channel so I wanted to bring this issue to the Expensify team's attention.

[sonialiap]

Thanks @MacroLens!

[sonialiap]

Brought up to the team

[rafecolton]

Looking

[rafecolton]

Thank you for your contribution. You have the right idea here and have correctly identified the bug, but your suggested fix will not work. Because we only do a shallow clone of main, there is almost certainly not enough history to identify the merge base, so git merge-base origin/main HEAD will return nothing. To fix this, we need to either remove the --depth flag on fetching main or change the value to some arbitrarily-large number. GH insights say there have been ~2500 commits in the last month so let's set depth to 30000. That should be more than enough to get the relevant history for a PR without getting the entire history.

@MacroLens I'm going to assign you, and provide some guidance here regarding what I am looking to see in the PR:

1. You'll have to go back a few commits when you check out your branch so there are relevant files changed in main, that you did not touch in your branch, that are failing lint checks. You should then edit a relevant file on your branch so that it has failing lint checks and submit a PR with your changes.
2. Then make the change to the script here. We should still see your file failing checks but the others from main should now be excluded.
3. We need to ensure that if git merge-base ever returns empty string, that causes the check to fail. Similarly, if git diff returns an error, that must also cause the check to fail. I'm not sure how easy it will be to do this directly in package.json and keep it readable, so it may be necessary to move the check to its own script.

Let me know if you have any questions.