chore(tooling): repo-wide lint rollout#18
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds repo-wide linting: root ESLint config and devDependencies, real eslint scripts for JS/TS packages, SwiftLint and ktlint integration for iOS/Android (configs and package scripts), includes mobile workspaces, updates .gitignore, and CI workflow to run conditional mobile lint jobs. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant CI as GitHub Actions
participant Affected as affected job
participant Generated as generated-code job
participant JSLint as JS/TS Lint (turbo/pnpm)
participant iOSLint as lint-ios (macOS / SwiftLint)
participant AndroidLint as lint-android (Ubuntu / ktlint)
PR->>CI: push / PR event
CI->>Affected: run affected detection (dry-run lint)
Affected-->>CI: outputs mobile_ios, mobile_android
CI->>Generated: run generated-code job (includes pnpm turbo run lint --max-warnings=0)
Generated->>JSLint: run JS/TS lint across workspaces
alt mobile_ios == true
CI->>iOSLint: run lint-ios (swiftlint lint)
iOSLint-->>CI: result
end
alt mobile_android == true
CI->>AndroidLint: run lint-android (./gradlew ktlintCheck)
AndroidLint-->>CI: result
end
CI-->>PR: report status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@eslint.config.mjs`:
- Around line 24-39: The ESLint config block currently sets
languageOptions.sourceType = "module" while matching files: ["**/*.mjs",
"**/*.cjs", "**/*.js"], which will misparse CommonJS (.cjs) files; fix by
splitting this block into two blocks (one with files ["**/*.mjs","**/*.js"] and
sourceType: "module", and another with files ["**/*.cjs"] and
languageOptions.sourceType: "commonjs"), or remove "**/*.cjs" from the module
block and add a dedicated CommonJS block; update the files array and
languageOptions.sourceType values accordingly so .cjs files use "commonjs" and
.mjs/.js use "module".
In `@mobile/android/gradle/wrapper/gradle-wrapper.properties`:
- Around line 1-7: Update the Gradle distribution URL in
gradle-wrapper.properties so the project uses Gradle 8.7+ (AGP 8.5.2 requires at
least 8.7): change the distributionUrl value (the distributionUrl property in
gradle-wrapper.properties) to point to a Gradle 8.7 or newer distribution (for
example gradle-8.7-bin.zip) and keep the other wrapper properties unchanged to
ensure compatibility with Android Gradle Plugin 8.5.2.
🧹 Nitpick comments (4)
eslint.config.mjs (1)
29-37: Consider using theglobalsnpm package instead of manually listing Node.js globals.Manual global definitions are incomplete (missing
Buffer,setTimeout,setInterval,URL,__dirnamebehavior differences in ESM, etc.) and can drift. Theglobalspackage provides well-maintained presets:import globals from "globals"; // ... globals: { ...globals.node },.github/workflows/ci.yml (2)
50-55: Consider pinning SwiftLint version and adding permissions.Two concerns with this job:
brew install swiftlintinstalls whatever version is current, which can cause CI flakiness if a new SwiftLint release introduces rule changes. Consider pinning withbrew install swiftlint@<version>or using a dedicated GitHub Action.- As flagged by CodeQL, the workflow (or at minimum this job) lacks a
permissionsblock. Since it only needs to read source, addingpermissions: { contents: read }follows the principle of least privilege.Proposed fix
lint-ios: runs-on: macos-latest + permissions: + contents: read steps: - uses: actions/checkout@v4 - run: brew install swiftlint - - run: cd mobile/ios && swiftlint lint + - run: cd mobile/ios && swiftlint lint --strict
57-65: Consider caching Gradle dependencies for the Android lint job.Without caching, every CI run will re-download Gradle and its dependencies. The
actions/setup-java@v4action supports Gradle caching natively.Proposed fix
- uses: actions/setup-java@v4 with: distribution: "temurin" java-version: "17" + cache: "gradle" + cache-dependency-path: mobile/android/*.gradle.kts - run: cd mobile/android && ./gradlew ktlintCheckapps/cms/package.json (1)
8-8: Inconsistent use of--no-error-on-unmatched-pattern.This is the only workspace using
--no-error-on-unmatched-pattern. While it's useful if the CMS currently has no lintable source files, it could silently mask misconfiguration (e.g., ESLint ignoring all files due to a glob issue). Consider adding a comment explaining why this flag is needed here, or removing it once the CMS has real source files.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In @.github/workflows/ci.yml:
- Around line 87-94: Update the lint-ios job to fail on SwiftLint warnings by
changing the swiftlint invocation in the step that currently runs "cd mobile/ios
&& swiftlint lint" to include the strict flag (use "swiftlint lint --strict")
and add a job-level permissions block that restricts GITHUB_TOKEN to read-only
repository contents (permissions: contents: read) so the job only has the
minimal scope it needs; locate the lint-ios job and the step that runs swiftlint
and update those two places accordingly.
- Around line 96-106: The lint-android job lacks a permissions block limiting
its repo access; add a permissions entry for the lint-android job (job name
"lint-android") that sets contents: read so the job only has read-only access to
repository contents before the steps run; update the job definition (the
lint-android job block) to include this permissions setting alongside
runs-on/needs/if/steps.
- Around line 53-57: The CI step silences errors by redirecting stderr to
/dev/null for the turbo and jq commands when computing AFFECTED, causing
failures to be hidden and mobile lint gating to be skipped; update the AFFECTED
computation (the line invoking "TURBO_SCM_BASE=\"$BASE\" pnpm turbo run lint
--affected --dry-run=json" and the subsequent "jq" pipeline) to capture and
surface stderr (e.g., write stderr to a temp file or pipe it to tee) and only
fall back to an empty value after logging the error, and ensure the script
writes diagnostic output before using AFFECTED to set "mobile_ios" and
"mobile_android" to avoid silent skips in CI.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In @.github/workflows/ci.yml: - Around line 96-106: The lint-android job lacks a permissions block limiting its repo access; add a permissions entry for the lint-android job (job name "lint-android") that sets contents: read so the job only has read-only access to repository contents before the steps run; update the job definition (the lint-android job block) to include this permissions setting alongside runs-on/needs/if/steps. - Around line 53-57: The CI step silences errors by redirecting stderr to /dev/null for the turbo and jq commands when computing AFFECTED, causing failures to be hidden and mobile lint gating to be skipped; update the AFFECTED computation (the line invoking "TURBO_SCM_BASE=\"$BASE\" pnpm turbo run lint --affected --dry-run=json" and the subsequent "jq" pipeline) to capture and surface stderr (e.g., write stderr to a temp file or pipe it to tee) and only fall back to an empty value after logging the error, and ensure the script writes diagnostic output before using AFFECTED to set "mobile_ios" and "mobile_android" to avoid silent skips in CI..github/workflows/ci.yml (2)
96-106: Addpermissionsblock tolint-android.Same CodeQL observation — this job only needs
contents: read.Suggested fix
lint-android: runs-on: ubuntu-latest needs: affected if: needs.affected.outputs.mobile_android == 'true' + permissions: + contents: read steps:🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In @.github/workflows/ci.yml around lines 96 - 106, The lint-android job lacks a permissions block limiting its repo access; add a permissions entry for the lint-android job (job name "lint-android") that sets contents: read so the job only has read-only access to repository contents before the steps run; update the job definition (the lint-android job block) to include this permissions setting alongside runs-on/needs/if/steps.
53-57:2>/dev/nullon bothturboandjqsilently swallows errors, causing mobile lint to be skipped on failure.If turbo or jq encounters a real error (e.g., JSON schema change, turbo misconfiguration), the
|| echo ""fallback setsAFFECTEDto empty, both outputs become"false", and mobile lint jobs are silently skipped. Consider redirecting stderr to a file or logging it, so genuine failures are visible in CI logs.Suggested approach
- AFFECTED=$(TURBO_SCM_BASE="$BASE" pnpm turbo run lint --affected --dry-run=json 2>/dev/null | jq -r '[.tasks[].package] | map(select(. == "@forge/mobile-ios" or . == "@forge/mobile-android")) | .[]' 2>/dev/null || echo "") + AFFECTED=$(TURBO_SCM_BASE="$BASE" pnpm turbo run lint --affected --dry-run=json 2>&1 | jq -r '[.tasks[].package] | map(select(. == "@forge/mobile-ios" or . == "@forge/mobile-android")) | .[]' || echo "")🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In @.github/workflows/ci.yml around lines 53 - 57, The CI step silences errors by redirecting stderr to /dev/null for the turbo and jq commands when computing AFFECTED, causing failures to be hidden and mobile lint gating to be skipped; update the AFFECTED computation (the line invoking "TURBO_SCM_BASE=\"$BASE\" pnpm turbo run lint --affected --dry-run=json" and the subsequent "jq" pipeline) to capture and surface stderr (e.g., write stderr to a temp file or pipe it to tee) and only fall back to an empty value after logging the error, and ensure the script writes diagnostic output before using AFFECTED to set "mobile_ios" and "mobile_android" to avoid silent skips in CI.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
docs: document lint commands in CONTRIBUTING Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… documentation - Added mobile lint jobs for iOS and Android to the CI workflow. - Updated lint command documentation in CONTRIBUTING.md for clarity. - Adjusted pnpm workspace to include mobile packages. - Modified pnpm-lock.yaml to reflect changes in mobile dependencies.
Co-authored-by: Cursor <cursoragent@cursor.com>
e0f189c to
e07e5e5
Compare
Review comments addressed
|
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In @.github/workflows/ci.yml:
- Around line 56-60: The workflow currently swallows all stderr from the
Turborepo dry-run and jq (the lines that set BASE and AFFECTED), which masks
real failures; modify the AFFECTED detection so the stderr from `pnpm turbo run
lint --affected --dry-run=json` and `jq` is captured and checked (e.g., redirect
stderr to a temp file or capture output/exit code), verify `pnpm turbo ...`
exits successfully before parsing, and if parsing fails emit a clear error and
fail the job instead of defaulting to false; ensure you still handle the
legitimate "no affected packages" case by detecting an empty JSON result from
the `pnpm turbo ...` output rather than relying on stderr suppression.
- Line 46: Three jobs ("affected", "lint-ios", and "lint-android") are using
actions/checkout@v4 while the rest use `@v6`; update the checkout step in the
affected job definitions (look for the actions/checkout usage in the "affected",
"lint-ios", and "lint-android" job blocks) to use actions/checkout@v6 so all
jobs are pinned to the same version.
In `@apps/cms/package.json`:
- Line 8: The lint script in apps/cms's package.json currently uses the
--no-error-on-unmatched-pattern flag which masks misconfigured file globs; when
the CMS app contains real JS/TS source files, remove that flag by updating the
"lint" script entry (the "lint" key in package.json) to use plain "eslint ." so
eslint will error on unmatched patterns and surface configuration/file-match
problems like the other packages.
In `@eslint.config.mjs`:
- Around line 16-23: The current global rule config applies
"@typescript-eslint/no-unused-vars" with argsIgnorePattern "^_" but that only
affects TypeScript-parsed files; add a parallel rule override for plain JS files
to mirror the same behavior by setting the base "no-unused-vars" rule for files
matching "*.js", "*.cjs", "*.mjs" (or the existing JS override block), and
configure it with "error" and argsIgnorePattern "^_" so underscore-prefixed
unused parameters are ignored in non-TypeScript files as well.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In @.github/workflows/ci.yml: - Line 46: Three jobs ("affected", "lint-ios", and "lint-android") are using actions/checkout@v4 while the rest use `@v6`; update the checkout step in the affected job definitions (look for the actions/checkout usage in the "affected", "lint-ios", and "lint-android" job blocks) to use actions/checkout@v6 so all jobs are pinned to the same version. In `@apps/cms/package.json`: - Line 8: The lint script in apps/cms's package.json currently uses the --no-error-on-unmatched-pattern flag which masks misconfigured file globs; when the CMS app contains real JS/TS source files, remove that flag by updating the "lint" script entry (the "lint" key in package.json) to use plain "eslint ." so eslint will error on unmatched patterns and surface configuration/file-match problems like the other packages..github/workflows/ci.yml (1)
46-46: Inconsistentactions/checkoutversions across jobs.The
affected,lint-ios, andlint-androidjobs useactions/checkout@v4(lines 46, 95, 104) while all other jobs use@v6. Pin all jobs to the same version to avoid divergence and simplify future upgrades.Proposed fix (apply to lines 46, 95, and 104)
- - uses: actions/checkout@v4 + - uses: actions/checkout@v6🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In @.github/workflows/ci.yml at line 46, Three jobs ("affected", "lint-ios", and "lint-android") are using actions/checkout@v4 while the rest use `@v6`; update the checkout step in the affected job definitions (look for the actions/checkout usage in the "affected", "lint-ios", and "lint-android" job blocks) to use actions/checkout@v6 so all jobs are pinned to the same version.apps/cms/package.json (1)
8-8: Consider dropping--no-error-on-unmatched-patternonce CMS has real source files.The flag is reasonable while the app is still a placeholder, but other workspace packages (
contracts,content-models,codegen) use plaineslint .. Once CMS gains actual TS/JS sources, remove the flag so a misconfigured file-match is surfaced rather than silently ignored.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@apps/cms/package.json` at line 8, The lint script in apps/cms's package.json currently uses the --no-error-on-unmatched-pattern flag which masks misconfigured file globs; when the CMS app contains real JS/TS source files, remove that flag by updating the "lint" script entry (the "lint" key in package.json) to use plain "eslint ." so eslint will error on unmatched patterns and surface configuration/file-match problems like the other packages.
| run: | | ||
| BASE="${{ github.event.pull_request.base.sha || github.event.before }}" | ||
| AFFECTED=$(TURBO_SCM_BASE="$BASE" pnpm turbo run lint --affected --dry-run=json 2>/dev/null | jq -r '[.tasks[].package] | map(select(. == "@forge/mobile-ios" or . == "@forge/mobile-android")) | .[]' 2>/dev/null || echo "") | ||
| echo "mobile_ios=$(echo "$AFFECTED" | grep -q "@forge/mobile-ios" && echo "true" || echo "false")" >> $GITHUB_OUTPUT | ||
| echo "mobile_android=$(echo "$AFFECTED" | grep -q "@forge/mobile-android" && echo "true" || echo "false")" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Suppressing stderr hides real failures in affected detection.
Both 2>/dev/null redirections (on turbo and jq) silently swallow errors. If Turborepo has a configuration issue, jq is missing, or the JSON schema changes, the job will quietly set both outputs to false and skip mobile linting without any CI signal.
Consider redirecting stderr to a file or using conditional logic so that unexpected failures surface while still tolerating the expected "no affected packages" case:
Proposed fix
- id: affected
run: |
BASE="${{ github.event.pull_request.base.sha || github.event.before }}"
- AFFECTED=$(TURBO_SCM_BASE="$BASE" pnpm turbo run lint --affected --dry-run=json 2>/dev/null | jq -r '[.tasks[].package] | map(select(. == "@forge/mobile-ios" or . == "@forge/mobile-android")) | .[]' 2>/dev/null || echo "")
+ TURBO_OUT=$(TURBO_SCM_BASE="$BASE" pnpm turbo run lint --affected --dry-run=json 2>&1) || true
+ AFFECTED=$(echo "$TURBO_OUT" | jq -r '[.tasks[].package] | map(select(. == "@forge/mobile-ios" or . == "@forge/mobile-android")) | .[]' 2>&1 || echo "")
echo "mobile_ios=$(echo "$AFFECTED" | grep -q "@forge/mobile-ios" && echo "true" || echo "false")" >> $GITHUB_OUTPUT
echo "mobile_android=$(echo "$AFFECTED" | grep -q "@forge/mobile-android" && echo "true" || echo "false")" >> $GITHUB_OUTPUT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| BASE="${{ github.event.pull_request.base.sha || github.event.before }}" | |
| AFFECTED=$(TURBO_SCM_BASE="$BASE" pnpm turbo run lint --affected --dry-run=json 2>/dev/null | jq -r '[.tasks[].package] | map(select(. == "@forge/mobile-ios" or . == "@forge/mobile-android")) | .[]' 2>/dev/null || echo "") | |
| echo "mobile_ios=$(echo "$AFFECTED" | grep -q "@forge/mobile-ios" && echo "true" || echo "false")" >> $GITHUB_OUTPUT | |
| echo "mobile_android=$(echo "$AFFECTED" | grep -q "@forge/mobile-android" && echo "true" || echo "false")" >> $GITHUB_OUTPUT | |
| run: | | |
| BASE="${{ github.event.pull_request.base.sha || github.event.before }}" | |
| TURBO_OUT=$(TURBO_SCM_BASE="$BASE" pnpm turbo run lint --affected --dry-run=json 2>&1) || true | |
| AFFECTED=$(echo "$TURBO_OUT" | jq -r '[.tasks[].package] | map(select(. == "@forge/mobile-ios" or . == "@forge/mobile-android")) | .[]' 2>&1 || echo "") | |
| echo "mobile_ios=$(echo "$AFFECTED" | grep -q "@forge/mobile-ios" && echo "true" || echo "false")" >> $GITHUB_OUTPUT | |
| echo "mobile_android=$(echo "$AFFECTED" | grep -q "@forge/mobile-android" && echo "true" || echo "false")" >> $GITHUB_OUTPUT |
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In @.github/workflows/ci.yml around lines 56 - 60, The workflow currently
swallows all stderr from the Turborepo dry-run and jq (the lines that set BASE
and AFFECTED), which masks real failures; modify the AFFECTED detection so the
stderr from `pnpm turbo run lint --affected --dry-run=json` and `jq` is captured
and checked (e.g., redirect stderr to a temp file or capture output/exit code),
verify `pnpm turbo ...` exits successfully before parsing, and if parsing fails
emit a clear error and fail the job instead of defaulting to false; ensure you
still handle the legitimate "no affected packages" case by detecting an empty
JSON result from the `pnpm turbo ...` output rather than relying on stderr
suppression.
| { | ||
| rules: { | ||
| "@typescript-eslint/no-unused-vars": [ | ||
| "error", | ||
| { argsIgnorePattern: "^_" }, | ||
| ], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
argsIgnorePattern may not apply to plain JS files.
The custom @typescript-eslint/no-unused-vars rule (lines 18-21) has no files filter, so it's set globally. However, for .js/.mjs/.cjs files that aren't processed by the TypeScript parser, this rule is effectively a no-op — the base no-unused-vars from js.configs.recommended applies instead, without the argsIgnorePattern: "^_" relaxation. Any JS file using _-prefixed unused params will still trigger an error.
If you want consistent behavior, add a parallel override for the base rule on JS files:
Proposed fix (add to the .mjs/.js block or as a new block)
{
files: ["**/*.mjs", "**/*.js"],
languageOptions: {
ecmaVersion: "latest",
sourceType: "module",
globals: {
console: "readonly",
process: "readonly",
__dirname: "readonly",
__filename: "readonly",
},
},
+ rules: {
+ "no-unused-vars": ["error", { argsIgnorePattern: "^_" }],
+ },
},
{
files: ["**/*.cjs"],
languageOptions: {
ecmaVersion: "latest",
sourceType: "commonjs",
globals: {
console: "readonly",
process: "readonly",
__dirname: "readonly",
__filename: "readonly",
module: "readonly",
require: "readonly",
exports: "writable",
},
},
+ rules: {
+ "no-unused-vars": ["error", { argsIgnorePattern: "^_" }],
+ },
},🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@eslint.config.mjs` around lines 16 - 23, The current global rule config
applies "@typescript-eslint/no-unused-vars" with argsIgnorePattern "^_" but that
only affects TypeScript-parsed files; add a parallel rule override for plain JS
files to mirror the same behavior by setting the base "no-unused-vars" rule for
files matching "*.js", "*.cjs", "*.mjs" (or the existing JS override block), and
configure it with "error" and argsIgnorePattern "^_" so underscore-prefixed
unused parameters are ignored in non-TypeScript files as well.
…rkflow Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Review feedback addressed (0bee771)CI workflow:
ESLint:
Not changed:
|
Resolves #3
Summary
Repo-wide lint baseline for JS/TS and mobile. Root ESLint flat config with explicit exclusion for
packages/clients/**and build artifacts. Replaced placeholder lint in cms and ai-orchestrator; added lint scripts to contracts, content-models, tooling-codegen. SwiftLint for iOS, ktlint for Android. CI uses--max-warnings=0for pnpm lint; new jobs for mobile lint.Contracts Changed
Regeneration Required
Validation
Summary by CodeRabbit
New Features
Documentation
Chores