chore: re-enable svelte-check in pre-commit for commit files only - #569
chore: re-enable svelte-check in pre-commit for commit files only#569ljodea wants to merge 5 commits into
Conversation
Add scripts/svelte-check-staged.mjs to run full svelte-check but only block commits when errors appear in staged paths. Parse machine-verbose output with timestamp-prefixed JSON records, fail on tool errors, and skip when all targets are deleted. Untrack generated .svelte-kit/tsconfig.json so svelte-kit sync in the hook does not dirty the index. CI still runs full pnpm check.
Address Codex review: fresh checkouts need generated tsconfig before type-aware oxlint (pre-commit and lint:oxlint). Normalize diagnostic filenames to posix slashes when matching staged targets.
✅ Deploy Preview for svelteplot ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db9475cfa4
ℹ️ 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".
|
|
||
| /** @param {string} filePath */ | ||
| export function toPosixPath(filePath) { | ||
| return filePath.split(path.sep).join('/'); |
There was a problem hiding this comment.
When pnpm test:scripts is run on Linux/macOS, path.sep is /, so toPosixPath('packages\\svelteplot\\...') leaves the backslashes intact. The new Windows-path unit test consequently fails because filterDiagnostics returns no match, and the same normalization gap would cause any backslash-separated diagnostic filename to be ignored rather than blocking the commit. Normalize \\ explicitly instead of relying on the host separator.
Useful? React with 👍 / 👎.
db9475c to
5c6892e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c6892e0a9
ℹ️ 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".
| 'exec', | ||
| 'svelte-check', | ||
| '--tsconfig', | ||
| './tsconfig.json', |
There was a problem hiding this comment.
When the hook is invoked for a package-only staged file such as packages/svelteplot/tests/areaY.test.ts (matched by .pre-commit-config.yaml:14), this always runs svelte-check against the root SvelteKit tsconfig. The --tsconfig docs say only files matched by that config's files/include/exclude are diagnosed, while packages/svelteplot/tsconfig.json:15 is the config that includes the package src and tests; package test files that are not imported by the docs app can therefore have svelte-check errors without ever appearing in parsed.errors, so the commit passes. Please dispatch package paths to the package tsconfig or narrow the hook's matched files.
Useful? React with 👍 / 👎.
Root SvelteKit tsconfig does not include packages/svelteplot/tests, so svelte-check errors in package test files were invisible to the staged hook. Partition targets and run the matching tsconfig per group.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70f23fe2f4
ℹ️ 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".
|
|
||
| if (packageTargets.length > 0) { | ||
| plans.push({ | ||
| tsconfig: PACKAGE_TSCONFIG, |
There was a problem hiding this comment.
When a staged file is under packages/svelteplot/tests, this routes it to packages/svelteplot/tsconfig.json, but that config does not define the Vite aliases used by the tests. The tests import svelteplot/svelteplot/*, while packages/svelteplot/package.json points those package exports at dist, and no packages/svelteplot/dist is tracked, so a fresh checkout can fail svelte-check with module-resolution errors in the staged test before any real test change is checked. Please add equivalent paths for the package config or avoid applying this package plan to tests that rely on the package-name aliases.
Useful? React with 👍 / 👎.
Package tests import svelteplot via Vite aliases pointing at src, but svelte-check used package exports to dist (missing in fresh checkouts). Mirror the vite resolve aliases in compilerOptions.paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25c344cae1
ℹ️ 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".
There was a problem hiding this comment.
This pattern now invokes the staged checker for any .js, .ts, .mts, or .cts file, but root targets are always checked with svelte-check --tsconfig ./tsconfig.json; the Svelte docs for --tsconfig say only files matched by that config's files/include/exclude are diagnosed, and the generated SvelteKit config inherited here covers the app/test/vite paths rather than root scripts such as screenshot-examples.js. If one of those matching-but-excluded files is staged, the hook runs but no diagnostic can ever match the staged file, so pre-commit run --all-files and normal commits give false confidence; narrow the regex or add a separate plan/tsconfig for those paths.
Useful? React with 👍 / 👎.
Root scripts like scripts/*.js are not in the SvelteKit tsconfig include globs, so the staged hook could run without ever matching diagnostics. Filter targets to diagnosable paths and narrow the pre-commit files regex to the same scope.
1 participant
Summary
Re-enables
svelte-checkin pre-commit without blocking commits due to legacy errors in untouched files.scripts/svelte-check-staged.mjs: runs fullsvelte-check, parsesmachine-verboseoutput (timestamp-prefixed JSON records), and only fails when errors appear in commit files.svelte-kit/tsconfig.json(already gitignored) sosvelte-kit syncdoes not dirty the indexsvelte-kit syncbefore type-aware oxlint in pre-commit andlint:oxlintso fresh checkouts work without the tracked tsconfigpnpm checkon every PRBehavior
.tsfile, legacy errors elsewherepre-commit run --all-filesTesting
pnpm test:scripts— 11 tests (parser, filtering, path normalization)stack.tspasses;VoronoiMesh.sveltefails as expectedpre-commit run svelte-check-staged --files packages/svelteplot/src/transforms/stack.tspasses