Skip to content

ci: run on master, build core before typecheck, require Node 20 - #25

Open
ronzim wants to merge 1 commit into
masterfrom
ci/fix-pipeline
Open

ronzim wants to merge 1 commit into
masterfrom
ci/fix-pipeline

Conversation

@ronzim

@ronzim ronzim commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Summary

The last CI run (push to refactoring, 2026-03-02) failed on two jobs. Both causes reproduced locally:

  • Type Check: the job type-checks packages/app without building mappit-core first, so mappit-core and its declarations are not found (TS2307). Fixed by building the core before type-checking.
  • Test (Node 18): Vite 7 and electron-vite 5 require Node ^20.19.0 || >=22.12.0. Node 18 is dropped from the matrix (now [20, 22]), and engines and README are updated to Node ≥ 20.

Also, CI now triggers on master (now the default branch) instead of main/refactoring.

Verification (local, clean clone)

  • npm ci, eslint packages/core/src packages/app/src: 0 errors (3 unused-variable warnings)
  • npm run build + npm run test (core): 16 files, 155 tests passed
  • typecheck core and app: pass after building core; app fails with TS2307 without it

🤖 Generated with Claude Code

- Trigger CI on push and pull requests to master (the default branch),
  instead of main/refactoring.
- Type Check job: build mappit-core before type-checking the app, which
  imports its declarations from dist/.
- Drop Node 18 from the test matrix: Vite 7 and electron-vite 5 require
  Node >= 20.19. Update engines and README accordingly.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Copilot AI balanced review requested due to automatic review settings October 8, 2026 15:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The change is a low-risk, internally consistent CI/config/docs update, but final human review of the default-branch switch to master is warranted since CI triggers depend on the repository's actual default branch, which can't be verified from the diff.

0 open findings

What changed in this PR

This PR fixes the two failing CI jobs from the last run by correcting build ordering and Node version support. The Type Check job failed because packages/app was type-checked without first building packages/core (whose compiled declarations the app resolves against), causing TS2307. The Test (Node 18) job failed because Vite 7 and electron-vite 5 require Node ^20.19.0 || >=22.12.0. The PR also retargets CI onto master, the new default branch.

Changes:

  • Build packages/core before type-checking in the typecheck job to resolve mappit-core declarations.
  • Drop Node 18 from the test matrix (now [20, 22]) and bump engines/README/badge to Node ≥ 20.
  • Switch CI push/pull_request triggers from main/refactoring to master.
File Description
.github/​workflows/​ci.yml Retargets triggers to master, drops Node 18 from the test matrix, and adds a core build step before the app/core typecheck.
packages/​core/​package.json Raises the engines.node floor from >=18 to >=20.
README.md Updates the Node badge and prerequisites to Node ≥ 20 (20.19 noted for Vite 7 / electron-vite 5).

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

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