Skip to content

fix(git-hooks): scope pre-push lint to branch-changed files only - #2247

Merged
Tobbe merged 4 commits into
cedarjs:mainfrom
lisa-assistant:lisa/scoped-pre-push-lint
Jul 27, 2026
Merged

fix(git-hooks): scope pre-push lint to branch-changed files only#2247
Tobbe merged 4 commits into
cedarjs:mainfrom
lisa-assistant:lisa/scoped-pre-push-lint

Conversation

@lisa-assistant

@lisa-assistant lisa-assistant commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Problem

The pre-push hook runs yarn lint, which ESLints all of packages/. On a monorepo with 70+ packages this is slow (~minutes) and extremely memory-hungry — in containers with ~4GB RAM it OOM-kills consistently.

Fix

Instead of linting all packages, lint only the files changed in the branch vs main, using git diff main...HEAD. This reuses the same getBranchChangedFiles() helper, getFilesToLint() filter, and runEslint() function already used by the pre-commit hook for staged files.

The result: lint time is proportional to the size of your change, not the size of the repo. A branch touching one file lints one file.

The build still runs in full (yarn build) so cross-package breakage from dependency changes is still caught. Lint still runs after build (not in parallel) to preserve the existing guarantee that template ESLint configs can require() dist output.

@netlify

netlify Bot commented Jul 27, 2026

Copy link
Copy Markdown

👷 Deploy request for cedarjs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit c04c4f3

@github-actions github-actions Bot added this to the next-release-patch milestone Jul 27, 2026
@greptile-apps

greptile-apps Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The pre-push hook now lints only files changed against the local main branch while preserving build-before-lint ordering.

  • Validates that the local main ref exists and reports diff failures instead of silently skipping lint.
  • Routes template and create-cedar-rsc-app changes through their package-specific lint commands.
  • Updates pre-push tests to mock branch-diff discovery and verify lint ordering and build-failure behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains in the changed pre-push lint flow, and the previously reported issues are addressed by explicit Git-ref validation and package-aware lint routing.

Important Files Changed

Filename Overview
tasks/git-hooks/tasks.mts Adds checked branch-file discovery and scoped lint routing, resolving the previously reported silent skips, package-config bypasses, template TSX omission, and upstream-remote dependency.
tasks/git-hooks/tests/tasks.test.mts Adapts pre-push ordering tests to the scoped ESLint invocation and isolates them from local Git ref and remote configuration.

Reviews (7): Last reviewed commit: "refactor(git-hooks): fail fast with a cl..." | Re-trigger Greptile

@nx-cloud

nx-cloud Bot commented Jul 27, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit c04c4f3

Command Status Duration Result
nx run-many -t build:pack --exclude create-ceda... ✅ Succeeded 5s View ↗
nx run-many -t build ✅ Succeeded <1s View ↗
nx run-many -t build --output-style=stream ✅ Succeeded 2m 48s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-07-27 16:15:15 UTC

Comment thread tasks/git-hooks/tasks.mts Outdated
Comment thread tasks/git-hooks/tasks.mts Outdated
@lisa-assistant
lisa-assistant force-pushed the lisa/scoped-pre-push-lint branch from 8f03a58 to 23c70ad Compare July 27, 2026 13:01
@lisa-assistant

Copy link
Copy Markdown
Contributor Author

Fixed both issues from Greptile:

  1. Missing main refgetBranchChangedFiles() now fetches main from upstream if it doesn't exist locally, ensuring the diff always succeeds instead of silently skipping lint.

  2. Package-specific lint configs — Added runAllLint() which detects template/crwrsca changes and calls their dedicated lint:templates and lint:crwrsca commands instead of routing them through the root ESLint config.

Comment thread tasks/git-hooks/tasks.mts Outdated
Comment thread tasks/git-hooks/tasks.mts
@lisa-assistant
lisa-assistant force-pushed the lisa/scoped-pre-push-lint branch from 23c70ad to ff0580f Compare July 27, 2026 13:16
@lisa-assistant

Copy link
Copy Markdown
Contributor Author

Fixed three additional P1 issues:

  1. Failed retry silently skips lint — Now checks fetch and retry diff status; throws error if either fails instead of silently swallowing stderr.

  2. Template TSX changes bypass lint — Moved template/crwrsca change detection to run on original changedFiles list before filtering, since getFilesToLint() excludes template .tsx files by design.

  3. Package-specific lint configs bypassed — Covered by the new runAllLint() logic that routes to appropriate lint commands based on which packages changed.

Comment thread tasks/git-hooks/tasks.mts Outdated
Running `yarn lint` on all packages on every push was slow and
memory-intensive (OOM in containers). The pre-commit hook already
lints staged files; the pre-push hook only needs to catch anything
not covered there.

Switch to linting only files changed in the branch vs main using
`git diff main...HEAD`, using the same `runEslint()` / `getFilesToLint()`
helpers already used by the pre-commit hook.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lisa-assistant
lisa-assistant force-pushed the lisa/scoped-pre-push-lint branch from ff0580f to 0f505af Compare July 27, 2026 13:31
@lisa-assistant

Copy link
Copy Markdown
Contributor Author

Fixed: getBranchChangedFiles() now tries multiple remotes (upstream, then origin) instead of hardcoding upstream. Falls back with a clear error message if main can't be fetched from any remote.

Lisa added 3 commits July 27, 2026 17:31
… CI git state

The test called the real (unmocked) getBranchChangedFiles(), which shells
out to `git diff main...HEAD`. On CI's shallow PR checkout there's no local
`main` ref and no `upstream` remote, so the fetch-fallback logic threw,
failing the test on both ubuntu-latest and windows-latest. Mock
node:child_process's spawnSync so the test is deterministic and update the
assertions to look for the `yarn eslint` call instead of the old `yarn lint`
command, which no longer exists after scoping lint to branch-changed files.
The comment claimed the branch-wide diff "catches anything not yet
committed", but getBranchChangedFiles() only diffs committed changes
(main...HEAD) — it never touches the working tree. Clarify the actual
reason for linting the whole branch diff instead of just the current
commit: pre-commit only lints per-commit staged files and can be
skipped with --no-verify, so some committed changes may slip through.
…missing

Replace the multi-remote fetch-and-retry fallback in
getBranchChangedFiles() with a simple upfront check
(`git rev-parse --verify main`). Silently fetching main from a remote
inside a git hook was fragile (network-dependent, remote-name-dependent)
and was the root cause of a prior CI test failure. Now we just tell the
developer exactly what to run instead.
@Tobbe
Tobbe merged commit 33a036c into cedarjs:main Jul 27, 2026
43 checks passed
@lisa-assistant

Copy link
Copy Markdown
Contributor Author

These P1 comments are all on earlier commits that I've now superseded with a simpler approach. The latest commit (c04c4f3) removes the multi-remote fetch-and-retry logic entirely and replaces it with a straightforward upfront check (git rev-parse --verify main) that either succeeds or fails with a clear, actionable error message telling the developer exactly what to run (git fetch origin main:main or upstream for forks).

This eliminates all three issues Greptile flagged:

  • No more silent failures on fetch or retried diff
  • No more template changes bypassing lint
  • No more network-dependent or remote-name-dependent blocking logic

CI has already validated the fix with all checks passing (including both build-lint-test jobs on ubuntu and windows that were previously failing).

@github-actions

Copy link
Copy Markdown

The changes in this PR are now available on npm.

Try them out by running yarn cedar upgrade -t 6.0.0-canary.2782

Or try it in a new app with yarn dlx create-cedar-app@6.0.0-canary.2782

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