ci: make PR checks actually run on shared-package changes - #184
Merged
Merged
Conversation
Every job in pr-checks.yml is gated behind a dorny/paths-filter, and none of the filters covered the repo's root packages/ directory. A PR touching only packages/ therefore skipped Lint, Type Check, Unit Tests and Build Check. Measured on PR #181, which changes two tsconfigs under packages/: 12 of its checks reported SKIPPED and nothing compiled the code it changed. - packages: also match packages/**, pnpm-workspace.yaml, pnpm-lock.yaml - web: matched frontend/apps/web/**, which does not exist; the web surface is the root web/ directory The mobile: and desktop: filters matched four paths that are absent from the repo -- frontend/platforms/{capacitor,react-native,electron,tauri}. The real directories are frontend/platforms/mobile and desktop/. Those two filters could never match, so Mobile Build Check and Desktop Build Check have never produced a verdict; had they matched, both would have failed, because each cds into one of the missing directories. Both jobs duplicate a dedicated workflow that already targets the real path: build-capacitor.yml builds frontend/platforms/mobile, and desktop-linux.yml, desktop-macos.yml and desktop-windows.yml build desktop/. Removing the two dead jobs drops no coverage that existed. Still uncovered and deliberately not papered over here: frontend-vite/ matches no filter, but every job in this workflow runs with working-directory: frontend, so adding a filter for it would run the wrong app's build and report a green that means nothing. It needs its own job.
ci.yml runs on push to main and gates its jobs with the same paths-filter shape as pr-checks.yml, including the same 'packages:' entry that only matched frontend/. A push touching only the shared packages/ directory skipped lint, type-check, test and build on main as well as on PRs. frontend/pnpm-workspace.yaml does not exist; the workspace manifest is at the repo root.
Security Scan Results: PASSED
|
PR Checks Summary
✅ All checks passed! Ready for review. |
Security Scan Results: PASSED
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
Every job in
pr-checks.ymlsits behind adorny/paths-filter, and none of the filters covered the repo's rootpackages/directory.Measured, not inferred — PR #181 changes exactly two files,
packages/ui/tsconfig.jsonandpackages/state/tsconfig.json:Nothing compiled the code that PR changes.
ci.yml, the push gate formain, carried the same hole in its ownpackages:filter.Dead filters
The
mobile:anddesktop:filters matched four paths that are absent from this repo:main?frontend/platforms/capacitor/**frontend/platforms/react-native/**frontend/platforms/electron/**frontend/platforms/tauri/**The real directories are
frontend/platforms/mobileanddesktop/. SoMobile Build CheckandDesktop Build Checkcould never trigger — and had they triggered they would have failed, because eachcds into one of those missing directories.Changes
packages:also matchespackages/**,pnpm-workspace.yaml,pnpm-lock.yaml(in bothpr-checks.ymlandci.yml)web:matchedfrontend/apps/web/**(does not exist) →web/**mobile:/desktop:filters, their job outputs, and the two dead jobsNo gate is weakened. Two jobs that reported nothing are removed, and four jobs that never ran on shared-package changes now do.
Correction to an earlier version of this description
I first wrote that both removed jobs "duplicate a dedicated workflow that already targets the real path" and so removing them "drops no coverage that existed". The second half is right; the first half was only half right, and I checked it properly afterwards.
Desktop — claim holds.
desktop-linux.yml,desktop-macos.ymlanddesktop-windows.ymlhave realpull_requesttriggers and builddesktop/. Desktop is genuinely covered at PR time (and #181 widens those triggers topackages/**).Mobile — claim was wrong.
build-capacitor.ymlisworkflow_dispatch+workflow_callonly, and nothing in the repo calls it. The same is true ofbuild-electron.yml,build-react-native.yml,build-tauri.ymlanddesktop-build.yml— five reusable workflows that no workflow invokes. So it is not the substitute I said it was.The real automatic mobile coverage is the
android-buildjob ("Android · debug APK (Capacitor)") inci.yml, which is ungated and runs on every push that triggers that workflow. Butci.ymlis push-only, so:Deleting
Mobile Build Checkdoes not cause that — the job could never run, so the gap already existed and was hidden by a job name that looked like coverage. I am flagging it rather than fixing it here, because giving mobile a real PR gate is a separate change with its own runtime cost.Left open on purpose
frontend-vite/matches no filter. I did not add one: every job in this workflow runs withworking-directory: frontend, so a filter forfrontend-vite/would build the wrong app and report a green that means nothing. It needs its own job.