Skip to content

Add path filtering to TS workflows to skip them for Elixir-only changes#4069

Open
alco wants to merge 7 commits intomainfrom
erik/ts-workflow-path-filtering
Open

Add path filtering to TS workflows to skip them for Elixir-only changes#4069
alco wants to merge 7 commits intomainfrom
erik/ts-workflow-path-filtering

Conversation

@alco
Copy link
Copy Markdown
Member

@alco alco commented Mar 27, 2026

Summary

  • Skip TS workflows on PRs that only touch Elixir packages, integration tests, or READMEs
  • Add changeset-release/main to push triggers
  • Add push trigger to ts_check_formatting.yml for main and changeset-release/main
  • Pushes to main and changeset-release/main always run TS CI (no path filtering)

Approach

Uses paths-ignore to exclude the few non-TS paths rather than an allowlist of every TS-related path. Since the repo is majority TS, the ignore list is shorter and doesn't need updating when new TS packages or config files are added.

Ignored paths:

  • packages/sync-service/**, packages/electric-telemetry/**, packages/elixir-client/**
  • integration-tests/**
  • **/README.md
  • website/** (in ts_tests.yml only — formatting covers website)

Branch protection note

GitHub treats workflows skipped due to path filtering the same as workflows where all jobs are skipped — required status checks from these workflows will not block merge for Elixir-only PRs.

Test plan

  • Verify this PR triggers TS CI (workflow files are not in the ignore list)
  • Verify an Elixir-only PR does not trigger TS CI
  • Verify pushes to main always trigger TS CI

🤖 Generated with Claude Code

erik-the-implementer and others added 6 commits March 27, 2026 14:18
…kflows

Use GitHub's built-in paths filter on pull_request triggers to skip TS CI
for Elixir-only changes, instead of a custom check_ts_changes gate job
that calls the GitHub API.

- PR triggers use paths allowlist to only run for TS-related file changes
- Push triggers for main and changeset-release/main always run (no filter)
- ts_check_formatting now also runs on push to main/changeset-release/main
- No extra permissions (pull-requests: read) or API calls needed
Dependency patches via pnpm.patchedDependencies can affect TS client
behavior, so changes to patches/ should trigger TS CI.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Agent task markdown files don't need formatting checks.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

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

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: 4ea2695a38

ℹ️ 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".

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.67%. Comparing base (0b45e63) to head (43b165e).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4069   +/-   ##
=======================================
  Coverage   88.67%   88.67%           
=======================================
  Files          25       25           
  Lines        2438     2438           
  Branches      613      613           
=======================================
  Hits         2162     2162           
  Misses        274      274           
  Partials        2        2           
Flag Coverage Δ
packages/experimental 87.73% <ø> (ø)
packages/react-hooks 86.48% <ø> (ø)
packages/start 82.83% <ø> (ø)
packages/typescript-client 93.81% <ø> (ø)
packages/y-electric 56.05% <ø> (ø)
typescript 88.67% <ø> (ø)
unit-tests 88.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alco alco added the claude label Mar 27, 2026
@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude Code Review

Summary

This PR adds path filtering to TS CI workflows so they are skipped for Elixir-only PRs. Since the previous review, the implementation was simplified from an allowlist (paths) approach to a denylist (paths-ignore) approach. The change is well-scoped and correct.

What's Working Well

  • Simplifying to paths-ignore (denylist) is a good call: new TS packages added in future will automatically trigger CI without needing to be added to an explicit allowlist.
  • Push triggers now have no path filtering at all — all pushes to main and changeset-release/main run TS CI, which is the right trade-off (lower risk of a regression slipping through on merge).
  • Adding push trigger to ts_check_formatting.yml closes the gap where formatting regressions could merge undetected.
  • Adding changeset-release/main to the ts_tests.yml push trigger closes a similar gap for release branches.
  • The packages/sync-service/** exclusion concern raised by the bot was correctly dismissed by the author: sync-service changes will still be caught by push-to-main CI.
  • The .agent-tasks entry in .prettierignore is harmless cleanup.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None.

Suggestions (Nice to Have)

  • Flaky test still present: The Codecov report still shows test/client.test.ts > should fall back to long polling after 3 consecutive short SSE connections failing (AssertionError: expected 5 to be less than or equal to 4). This is unrelated to these workflow changes — worth confirming it's a known flaky test tracked elsewhere before merging.

Issue Conformance

No linked issue. Acceptable for a CI housekeeping change; the PR description explains the motivation clearly.

Previous Review Status

  • Resolved: packages/start/ coverage concern — no longer applicable with the denylist approach.
  • Resolved: Allowlist completeness concerns — not applicable with the denylist approach.
  • Remaining: Flaky test noted in previous review is still present in CI output (unrelated to this PR).

Review iteration: 2 | 2026-03-27

…wlist

The repo is majority TS with only a handful of Elixir packages, so ignoring
the few Elixir paths is simpler and more maintainable than enumerating every
TS-related path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alco alco self-assigned this Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants