Repository navigation
Conversation
…t config comments
…int-parity ignore
…-tightens on main
d935940 to
eee5353
Compare
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eee5353330
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9de42e92b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6679091351
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… settings instead of copying them
…into LukasMod/experiment-oxlint-parity-no-seatbelt
Details
Moves Onyx's linting from ESLint to Oxlint, using the rules from
eslint-config-expensify.oxlint.config.mtscallsbuildOxlintConfig()fromeslint-config-expensify(Expensify/eslint-config-expensify#214) with the presets Onyx uses, and adds only Onyx's own ignores, settings and overrides. New or changed rules come from bumpingeslint-config-expensify; no rules are copied here. Expensify/App#106218 does the same for App.eslint-config-expensifyis installed from that PR's commit on GitHub. Before merging, it gets swapped for the released version.import/namedandimport/exportrun as Oxlint's own rules. Onyx has no findings from either.oxlint.config.mtsnames a rule as ESLint spells it although it runs ascore/orhosted/, the config fails to load with a message saying what to change. Other wrong names fail with Oxlint's own error.eslint.config.mjsis removed andnpm run lintruns Oxlint 1.92 with type-aware rules. The ESLint packages stay installed because the presets and those rules need them.config/oxlint/oxlint.seatbelt.tsv, the same setup as in E/App. New errors fail CI, and fixing errors lowers the allowed count. After a merge, the lint job onmaincommits the lowered baseline as os-botify.naming-conventionlist, the TypeScriptno-unused-varsoptions) start fromgetPresetRuleEntry()from the package instead of copying the preset's settings.expensifypreset, as before. A few rules the old ESLint config set are restored inoxlint.config.mts:import/no-unresolvedon TypeScript files, andno-unused-varsagain accepts parameters named with a leading_. Three seatbelt rows for such parameters went away.oxlint.config.mtsand the scripts inconfig/oxlint/are linted too, with Node globals and the browser-compatibilityes/*rules turned off.tsconfig.jsonusesmoduleResolution: bundlerbecause tsgolint (type-aware Oxlint) rejectsnode.tsconfig.build.jsonand.github/actions/javascript/tsconfig.jsonkeep CommonJS, so the library build and the GitHub Action bundles come out as before.lib/only has comment updates and drops an unusedreact-native-nitro-sqlite/mocktype declaration.dist/built from this branch differs frommainonly in comment lines.19 of the 681 are in tests added on
mainafter the parity measurement below.main's ESLint config does not enableno-unsafe-assignment,no-unsafe-call,no-unsafe-member-access,no-unsafe-type-assertionorno-unnecessary-type-assertion, so ESLint never flagged them, while the oxlint config does.Parity with ESLint, measured before ESLint was removed: all 665 Oxlint findings match ESLint's. ESLint reported 48 more: 47
rulesdir/prefer-at(Oxlint usesunicorn/prefer-at, which catches fewer cases, as in E/App) and 1no-unnecessary-type-assertion. Moving tobuildOxlintConfigand Oxlint 1.92 kept the findings the same, apart from the 3_parameters above:SEATBELT_FROZEN=1 npm run lintpasses, also with the latest package commit.Related Issues
N/A, lint tooling change discussed with Rory in Slack.
Linked E/App PR
N/A, lint tooling only. No runtime code changes, so there is nothing to verify in E/App.
Automated Tests
config/oxlint/seatbelt.test.mjs: 19 tests for the seatbelt ratchet (npm run test:lint, added to the test workflow).test:typespass unchanged.Manual Tests
[No QA]: lint tooling only, no runtime changes. Local checks:
npm ci, thennpm run lint: prints0 errors.console.log('test');tolib/utils.tsand runnpm run lint: fails withno-console. Revert.config/oxlint/oxlint.seatbelt.tsv, raise one row's count by 1 and runCI=true npm run lint: passes and rewrites the row back to the real count (what themainjob commits). Revert.npm run test:lint: 19 tests pass.Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A, no UI or runtime changes.
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari