Skip to content

[No QA] Rebuild lint seatbelt pipeline outside ESLint - #99259

Draft
roryabraham wants to merge 15 commits into
mainfrom
rory/lint-pipeline
Draft

[No QA] Rebuild lint seatbelt pipeline outside ESLint#99259
roryabraham wants to merge 15 commits into
mainfrom
rory/lint-pipeline

Conversation

@roryabraham

@roryabraham roryabraham commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Rebuild the lint post-process pipeline (React Compiler filter, no-deprecated stratification, seatbelt ratchet) as linter-agnostic transforms in scripts/lint/, still driven by eslint --format json. This is Phase 3 / §4.1 of the Oxlint migration: drop eslint-seatbelt and its three patches, do one TSV read/write per run instead of ~8,400 locked per-file updates, and only run the React Compilers on files that actually have a suppressible message.

Measured on a full-repo dump (8,489 files, 3,479 messages): post-process is ~9s cold / ~230ms warm. Oxlint is out of scope.

Full-Repo Lint Benchmark

Repository Cold cache Warm cache Result
/Users/rory/Expensidev/App (baseline) 294.83s 26.30s PASS
/Users/rory/Expensidev/App-Worktrees/rory-lint-pipeline (PR) 145.39s 10.77s FAIL: 1 lint error

Method: each repository ran npm run lint sequentially. Before each cold run, node_modules/.cache/eslint was removed; the warm result is the immediately following run using the populated cache. Timings are wall-clock seconds from /usr/bin/time -p; each invocation had a 30-minute timeout. The PR worktree failed both runs consistently at src/libs/Navigation/helpers/swapBackgroundTabForRHPTarget.ts:41:74 with @typescript-eslint/no-unnecessary-type-assertion.

Editor integrations and bare npx eslint still run the rule set but no longer apply seatbelt / compiler filter / stratify — those live only in npm run lint. Documented in LINTING.md.

Fixed Issues

$
PROPOSAL:

Tests

  1. Run TZ=utc bun test --preload ./scripts/stubReactNative.js --preload ./tests/tooling/setup.ts ./tests/tooling/lintSeatbelt.test.ts ./tests/tooling/lintPipeline.test.ts and confirm all tests pass.
  2. Run TZ=utc NODE_OPTIONS="--experimental-vm-modules --max_old_space_size=8192" bunx jest tests/unit/OnyxConnectBypassTest.ts --no-coverage and confirm all tests pass.
  3. Run npm run lint -- scripts/lint scripts/checkOnyxConnectBypass.ts scripts/onyxConnectBypass.ts and confirm it exits 0.
  4. Run npm run lint -- --timings scripts/lint and confirm per-stage timings are printed.
  5. With SEATBELT_READ_ONLY unset (local default), run npm run lint -- scripts/lint/seatbelt.ts and confirm config/eslint/eslint.seatbelt.tsv is unchanged.
  6. Confirm package.json no longer lists eslint-seatbelt and patches/eslint-seatbelt/ is gone.
  • Verify that no errors appear in the JS console

Offline tests

This change is lint tooling only and has no runtime / network behavior.

QA Steps

Same as tests. Lint tooling only; no product QA.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • 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)
    • 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
  • 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 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 function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

Move react-compiler, no-deprecated, and seatbelt post-process
out of ESLint processors into scripts/lint so one JSON reduce
replaces ~8400 per-file TSV locks. Drop eslint-seatbelt and
its three patches. Replay dumps via --from-raw for benchmarks.
@roryabraham

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Code review

Nicely structured rewrite — the pipeline stages are cleanly separated, the seatbelt ratchet logic is faithfully reproduced, and I confirmed the committed eslint.seatbelt.tsv is already byte-sorted (matches the new serializer's default sort), so the first CI run won't churn the baseline. Three parallel reviewers went through it; one confirmed correctness issue stands out, plus a few lower-severity notes.

🔴 Blocker — a JSON parse failure exits 0 and passes CI silently

When ESLint's output can't be parsed and ESLint exited 0 or 1, the whole lint run reports zero errors, prints nothing, and exits 0.

  • eslint.ts:107 / :114 / :117 force linterExitCode to 1 on a parse failure — the clear intent being "we couldn't parse the linter, so fail."
  • But pipeline.ts:40 only takes the fatal early-return when linterExitCode > 1. 1 falls through to the normal path: flatten([]) → 0 messages → renderReporterrorCount 0exitCode 0.
  • index.ts:51 also gates the stderr diagnostic on > 1, so the Failed to parse ESLint JSON output message is never printed.

So the author's own "force a failure" safety net is dead code, and since CI runs if ! npm run lint, a broken lint invocation (polluted/truncated stdout, a plugin writing stray [...], OOM mid-stream) goes green. --dump-raw has the same hole — it writes an empty dump and a later --from-raw replays it as a clean pass. No test covers this: lintPipeline.test.ts only exercises linterExitCode: 2.

Fix: make parse failure fatal — return Math.max(2, result.exitCode) (or any value > 1) so the pipeline.ts:40 gate fires and the stderr diagnostic surfaces. Add a test for the parse-fail-with-exit-≤1 path.

🟡 Worth addressing

  • A single React Compiler worker exception aborts the entire lint. reactCompilerFilter.ts:89 rejects on any worker error, which propagates uncaught out of runPostprocess. One pathological file crashes linting of everything else instead of degrading to the conservative "not memoized → keep the message" default already used for missing map entries. Consider catching per-file worker errors and treating them as bothMemoized = false.
  • --from-raw doesn't fully reproduce a real run. It skips checkOnyxConnectBypass entirely (index.ts:67) and re-reads/re-runs the React Compiler against the current worktree rather than the captured source. It's documented as a benchmarking aid (LINTING.md), so this is low-impact, but the replay's pass/fail can differ from the captured run — worth a one-line caveat in the docs.

🔵 Minor / nits

  • Onyx-bypass counts are now per-directive, not per-message. onyxConnectBypass.ts:51 counts disable directives via regex; one block-disable that silenced N Onyx.connect() calls now counts as 1 instead of N. The ban still fails on ≥1 bypass so nothing escapes, but reported counts/lines changed vs. the ESLint-driven check. Also a stale rule-specific disable next to an approved Onyx.connectWithoutView would now be a false positive. Fine to accept — just flagging the semantic shift.
  • Frozen-mode guidance points at the wrong file. seatbelt.ts:396 passes the linted source file where the TSV path is expected, so the "then commit …" hint names the source file instead of eslint.seatbelt.tsv. Frozen-only (off by default).
  • Misleading prune log locally. seatbelt.ts:488 prints removed N baseline row(s) for deleted files even in read-only (local) mode where nothing is written — consider gating the log on shouldWrite or rewording to "would remove".
Things I verified as correct
  • Seatbelt ratchet: TSV parse/serialize round-trip (header comments preserved, JSON-encoded counts, tab-safe), off-by-one demotion of the first maxErrorCount messages by canonical sort, zero-count rows removed, deleted-file rows pruned, atomic temp-file + rename write, deterministic output (committed TSV == default .sort() output → no spurious writes). Read-only design is readOnly: !env.CI (local read-only; CI writes and OSBotify commits the tightened baseline) — this matches the pre-PR ESLint config and does not weaken enforcement, because new violations stay severity: 2 regardless of readOnly and a baseline is never auto-increased without SEATBELT_INCREASE.
  • Stage order (react-compiler-filter → stratify → seatbelt → report) matches the old processor chain; stratifyNoDeprecated.ts is a near-exact port of the deleted processor; reactCompilerFilter.ts reproduces the compat processor and only invokes the (expensive) compilers on files with a suppressible message.
  • No dangling references to eslint-seatbelt, the deleted processors, or old script paths; patches/eslint-seatbelt/ and config/eslint/processors/ are cleanly removed; both tsconfig.json files updated to include the new files.
  • Editor / bare npx eslint behavior change (seatbelt/compiler-filter/stratify now only in npm run lint, so grandfathered violations show as errors in editors) is accurately documented in LINTING.md.

This is lint-tooling only (no runtime/network behavior), so no App testing was run.


view run

JSON parse failures now exit >1 so CI cannot go green on
truncated ESLint output. Arg parsing uses expensify-common/CLI.
Compiler workers share a WorkerPool that falls back per file
instead of aborting the run.
Move wall-clock stage timing out of the lint package so other
scripts can reuse it.
expensify-common/CLI lives in the runner; seatbelt env parsing
belongs next to the ratchet.
Benchmark replay is not worth the extra CLI and pipeline paths.
Pipeline now composes abstract Linter, Processor, and Formatter
ports. ESLint, seatbelt, compiler filter, and stylish output
live in named folders so a new linter or stage is a new class.
@roryabraham

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review (head 6c5b15d)

Thanks for the rework — this is a clean improvement. All three findings from my last pass are resolved:

  • Blocker (silent green on parse failure) — fixed end-to-end. EslintLinter.ts:75 now returns Math.max(2, exitCode), LintPipeline.ts:32 treats >1 as fatal, and index.ts:79 propagates it. New tests in lintPipeline.test.ts cover it.
  • Worker exception aborting the whole lint — replaced by WorkerPool.ts with per-item fallback + a leftover safety net, and the React Compiler fallback correctly keeps the message (conservative direction — never silently suppresses).
  • --from-raw fidelity — moot; dump/from-raw was dropped entirely.

The Seatbelt move is mechanical (the +96 lines are the env-parsing helpers relocated from the deleted args.ts plus the Processor wrapper), and it even folds in the two frozen-mode/prune-log nits I raised. StylishFormatter computes counts identically to the old report.ts. Nothing below is blocking.

🔵 Minor

  • Flags after positional targets are silently dropped. expensify-common/CLI breaks the parse loop once the variadic targets starts collecting, so npm run lint -- src/foo.ts --fix silently ignores --fix (index.ts:42). The old args.ts accepted flags in any position. All in-repo callers put flags first so it's latent, but worth a one-line note in LINTING.md (flags before paths) — or it'll bite someone.
  • WorkerPool message-after-error race (WorkerPool.ts:61). If a worker posts a result and then errors, the error handler resolves but doesn't stop the message handler, so a stray pump() pops the next item and posts it to the now-dead worker — that item then falls to fallback. It's conservative (a clean file just keeps its React-Compiler message, no false pass) and needs a rare ordering, but a settled flag guarding both handlers closes it cheaply.
  • Worker test coverage gap. WorkerPool.test.ts uses items == poolSize, so the leftover-fallback loop (WorkerPool.ts:40) and the mixed success+crash path (one flaky worker while others finish) are never exercised — that partial-crash case is the main real-world scenario. Consider a test with more items than workers and one crashing worker.
  • Fatal linter output goes to stdout, not stderr (index.ts:71). The crash diagnostic is now printed via console.log; the old code used console.error. Exit code still propagates so CI fails correctly — purely cosmetic.

Nice cleanup overall. Lint-tooling only, so no App testing was run.


view run

@roryabraham

Copy link
Copy Markdown
Contributor Author

@codex review

@roryabraham

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3cd1aaeee

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread scripts/onyxConnectBypass.ts
Comment thread scripts/lint/processors/ReactCompilerFilter.ts Outdated
Comment thread scripts/lint/index.ts
@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review (updated at c3cd1aae)

Both blockers from the last round are resolved, and the ports/adapters restructure (Linter / Processor / Formatter + LintPipeline, extracted WorkerPool) reads well:

  • Parse failures now fail closedESLintLinter.ts:78-84 forces Math.max(2, exitCode), so the exitCode > 1 gate in LintPipeline.ts:32 fires and index.ts:72 surfaces stderr with a non-zero exit.
  • Worker exceptions are isolatedReactCompilerWorker.ts always replies (bothMemoized: false on throw) and WorkerPool falls back per-item, with tests covering single-worker death, total-pool death, and selective crash.

One new blocker below (a merge-gate failure), plus a few medium items worth a look.

🔴 Blocker — typecheck will fail on the new selective-crash worker fixture

tests/tooling/workerPoolSelectiveCrashWorker.ts:9 does declare const self: Worker; in a non-module (global-script) file, which collides with the DOM lib's global self. Its two sibling fixtures avoid this by being excluded from the root program — but this file was added only to tests/tooling/tsconfig.json and is missing from:

Since the root include contains "tests", the file is pulled into the root tsc program and hits the same self-redeclaration conflict your own comment (tsconfig.json:51-53) documents for the siblings — so npm run typecheck (the required merge gate) fails, and it's also linted against the wrong project. Fix: add tests/tooling/workerPoolSelectiveCrashWorker.ts to both the root exclude array and the eslint.config.mjs tests/tooling files list, mirroring its two siblings.

🟡 Worth addressing

  • WorkerPool can hang if a worker dies without an error event. WorkerPool.ts:47-91 only listens for message and error. A worker that OOM-kills, segfaults, or process.exits (or a messageerror structured-clone failure) leaves inFlight unsettled → drain never resolves → Promise.all never resolves → the whole lint hangs. This trades the old "one file aborts everything" for "one file hangs everything." Consider a per-task timeout (resolve via fallback + terminate/replace the worker) and/or handling the worker close/exit event.
  • Compiler cache key omits the file extension. ReactCompilerFilter.ts:150 / :169 key the cache on Bun.hash(source) alone, but the OXC memoization verdict depends on the extension (.tsx/.ts/.jsx parse differently). Two files with identical content but different extensions (or a .ts.tsx rename) collide and get the wrong verdict → a suppressible message wrongly dropped or kept, and it persists in node_modules/.cache. Fold the extension into the hash input.
  • Unguarded source read re-introduces "one file aborts the run." ReactCompilerFilter.ts:148 awaits file(filename).text() inside Promise.all with no try/catch. If a file that produced a message is deleted/edited mid-lint (or a cache write fails on a full disk), the whole processor rejects and aborts the lint — the exact failure class this PR set out to eliminate, just moved to the read stage. Wrap the per-file read/write and treat a failure as a conservative keep.
  • CLI flags after a target path are silently ignored. The targets positional is variadic (index.ts:42-49); expensify-common/CLI stops parsing argv once positionals begin, so npm run lint -- src/foo.ts --fix drops --fix with no error (the old args.ts scanned all of argv). Documented invocations put flags first so common paths are fine, but it's a footgun worth a note in LINTING.md. Relatedly, unknown flags now hard-fail rather than passing through to ESLint — arguably an improvement, but the "forwarding any user-provided flags" comment in lintChanged.sh:30 is now inaccurate.

🔵 Nits

  • Dead per-file sort in Seatbelt.ts:124 — a localeCompare sort fully overridden by the later global lines.sort() (code-unit). Harmless but misleading; drop it.
  • StylishFormatter.name = 'report' (StylishFormatter.ts:19) — carried over from report.ts; only a Bench label, but the name/class mismatch is confusing.
  • SEATBELT_INCREASE overriding SEATBELT_READ_ONLY (Seatbelt.ts:563) has no test pinning the precedence — worth one to lock it.
Verified correct / not blocking
  • Seatbelt ratchet invariants hold: new violations stay severity 2 regardless of readOnly; CI writes only ratchet down or prune (never auto-baseline an increase without SEATBELT_INCREASE); exact off-by-one demotion of the first maxErrorCount messages; zero-remaining rows removed; dead-file prune keyed on filesystem existence so partial (subset) writes are safe; steady-state run writes nothing (idempotent); global lines.sort() matches the committed order so no churn.
  • ruleIdruleID rename normalized at the ESLint boundary (ESLintLinter.ts:54); all downstream consumers use ruleID.
  • StylishFormatter counts only severity >= 2 as errors (demoted grandfathered errors count as warnings), drives the exit code correctly, and returns {text:'', 0, 0} on empty input.
  • StratifyNoDeprecated derives its per-API rule-ID suffix from source.slice(...), which is unstable across variable renames / reformatting — but this is a faithful port of the deleted eslint-processor-stratify-no-deprecated.mjs (identical slice + replaceAll(/[\s/]+/g,'_')), so it's pre-existing behavior, not a regression here.
  • No dangling references to --dump-raw / --from-raw / args.ts / pipeline.ts / report.ts / timings.ts / eslint-seatbelt in docs/scripts/package.json; WorkerPool task→result mapping is index-keyed (correct under out-of-order arrival), bounded to poolSize, and terminates workers in finally.

This is lint-tooling only (no runtime/network behavior), so no App testing was run.


view run

A silent worker exit could hang lint, and the new crash
fixture was missing from the root tsconfig exclude list.
@roryabraham

Copy link
Copy Markdown
Contributor Author

@codex review
@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review (head 69c36f9)

The two commits since my last pass (b2117567 harden bypass/cache, 69c36f9 close worker hang + typecheck hole) are a clean, well-targeted hardening pass. I traced each change and found no new issues — this looks ready to me.

What landed, and why it's correct:

  • Worker hang fixed. WorkerPool.ts:66 now routes error, messageerror, and close through a single fail() that settles the drain with a fallback. A worker that dies without emitting error (e.g. process.exit) previously left Promise.all pending forever; now it resolves. The settled guard means terminate()'s close on a healthy worker is a harmless no-op, and leftover queue items still fall back in map(), so nothing is dropped. The new workerPoolExitWorker.ts + Promise.race timeout test exercises exactly this path.

  • Cache invalidation fixed. The React Compiler cache key now folds in a compiler-config fingerprint (babel/oxc configs, package-lock.json, the filter/worker/pool sources) plus the file extension, so a compiler or version bump no longer serves stale memoization decisions from a content-only key. Read/write are wrapped conservatively (recompute/skip rather than abort), and an unreadable source now keeps the message (new test covers it).

  • Onyx-bypass hole closed. blanketDirectiveCoversCall makes a blanket eslint-disable count only when it actually covers a real Onyx.connect() call — correctly handling -line (same line), -next-line (next line), and block form up to a matching eslint-enable. Enabling an unrelated rule doesn't re-enable the ban, so no false negative; unrelated blanket comments (ReportUtils etc.) still don't count, so no false positive. Good balance.

  • Typecheck hole closed. workerPoolSelectiveCrashWorker.ts was in the ESLint overrides but absent from both tsconfig.json include lists (so it wasn't typechecked); it and the new exit worker are now in both.

  • Nice bonus: the redundant intra-file localeCompare sort I flagged in round 1 is gone; the global sort is the single source of order.

Every finding from the earlier rounds (silent-green-on-parse-failure blocker, worker-exception abort, cache staleness, blanket-disable escape) is resolved. No blockers or new concerns on my end. 👍

Lint-tooling only (no runtime/network behavior), so no App testing was run.


view run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

const output = execFileSync('git', ['grep', '-lI', '-F', '--all-match', '--untracked', '--no-recurse-submodules', '-e', 'Onyx.connect', '-e', 'eslint-disable', '--', ...pathSpecs], {

P1 Badge Include spaced Onyx calls in the candidate scan

When a caller writes the equivalent Onyx . connect(...) or splits the member access across lines, ESLint still reports the banned call and an adjacent disable can suppress it, but this fixed-string git grep does not select the file. The downstream ONYX_CONNECT_CALL_REGEX already accepts whitespace, yet it is never invoked, allowing npm run lint to pass a forbidden direct Onyx read; candidate selection should not require the contiguous text Onyx.connect. CLAUDE.mdL36-L38

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

git grep is line-oriented, so requiring contiguous
Onyx.connect skipped split member access.
@roryabraham

Copy link
Copy Markdown
Contributor Author

Addressed the latest Codex P1: candidate scan no longer requires contiguous Onyx.connect. git grep --all-match now looks for Onyx, connect, and eslint-disable so spaced/split calls still reach ONYX_CONNECT_CALL_REGEX. Test covers both forms.

@roryabraham

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8636ec2a22

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread scripts/onyxConnectBypass.ts Outdated
Comments and grouping parens are valid member-access
syntax and slipped past the source regex.
@roryabraham

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b416214e3e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread scripts/onyxConnectBypass.ts Outdated
Block comments can list the banned rule after the
first line; the old regex stopped at the newline.
@roryabraham

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: adcbc92eb4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread scripts/onyxConnectBypass.ts Outdated
Scan TypeScript comment trivia so string contents
cannot re-enable a disabled lint rule.
@roryabraham

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cc71b6b6e3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

const sourceFile = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
const offsets: number[] = [];
const visit = (node: ts.Node) => {
if (ts.isCallExpression(node) && ts.isPropertyAccessExpression(node.expression) && !node.expression.questionDotToken && node.expression.name.text === 'connect') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Detect optional Onyx.connect calls

A blanket directive can still bypass the ban when the call uses optional member access, for example /* eslint-disable */ Onyx?.connect(...): TypeScript represents this as a property-access call with questionDotToken, but this condition explicitly excludes it, so blanketDirectiveCoversCall sees no covered call while ESLint's disabled rule remains suppressed. Fresh evidence beyond the addressed AST fix is this explicit optional-chain exclusion; treat Onyx?.connect() like the other valid member-access spellings.

Useful? React with 👍 / 👎.

# Conflicts:
#	config/eslint/eslint.config.mjs
#	package-lock.json
#	package.json
#	scripts/tsconfig.json
#	tests/tooling/tsconfig.json
#	tsconfig.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants