Skip to content

Conversation

@bpowers
Copy link
Owner

@bpowers bpowers commented Jan 19, 2026

The pre-commit hook previously ran yarn tsc which does nothing
meaningful because the root tsconfig.json has "files": [] with only
project references. This meant TypeScript errors weren't being caught
locally but would fail in CI.

Changes:

  • Use yarn tsc --build to properly build all project references
  • When WASM modules aren't built, check independent projects (core,
    website, engine2, xmutil-js) that don't depend on WASM
  • Fix rspress.config.ts type error by migrating from deprecated
    analytics config to builderPlugins with rsbuild-plugin-google-
    analytics

The rspress 1.44.0 no longer supports the analytics top-level config
property. Google Analytics must now be configured via the
rsbuild-plugin-google-analytics plugin through the builderPlugins
config.

The pre-commit hook previously ran `yarn tsc` which does nothing
meaningful because the root tsconfig.json has `"files": []` with only
project references. This meant TypeScript errors weren't being caught
locally but would fail in CI.

Changes:
- Use `yarn tsc --build` to properly build all project references
- When WASM modules aren't built, check independent projects (core,
  website, engine2, xmutil-js) that don't depend on WASM
- Fix rspress.config.ts type error by migrating from deprecated
  `analytics` config to `builderPlugins` with rsbuild-plugin-google-
  analytics

The rspress 1.44.0 no longer supports the `analytics` top-level config
property. Google Analytics must now be configured via the
rsbuild-plugin-google-analytics plugin through the builderPlugins
config.
Add gitignore patterns for files generated by `yarn tsc --build`:
- *.tsbuildinfo files
- Generated .js, .js.map, .d.ts in src/engine/ and src/importer/
- Generated .js files in website/

These artifacts are created when running type checking with the
--build flag and should not be committed.
@bpowers bpowers closed this Jan 19, 2026
@claude
Copy link

claude bot commented Jan 19, 2026

Code Review for PR #191: build: fix pre-commit TypeScript type checking

Summary

This PR addresses a critical issue where TypeScript type checking was not actually running in the pre-commit hook. The root cause was using yarn tsc instead of yarn tsc --build when the root tsconfig.json only contains project references.

Strengths

  1. Correctly identifies and fixes the core issue: Using --build flag is the right approach for TypeScript project references
  2. Graceful degradation: The fallback to check independent projects when WASM is not built is a pragmatic solution for development workflows
  3. Comprehensive gitignore updates: Properly ignores all TypeScript build artifacts (*.tsbuildinfo, generated .js/.d.ts files)
  4. Fixes actual type error: The rspress.config.ts migration from deprecated analytics to builderPlugins shows the hook would have caught real issues

Observations and Suggestions

1. Pre-commit script logic (scripts/pre-commit:80-147)

Good: The script checks for WASM availability and handles both cases appropriately.

Minor concern: The script checks 4 independent projects (core, website, engine2, xmutil-js) when WASM is not built, but it is unclear if this covers all possible TypeScript errors.

  • src/diagram, src/app, and src/server appear to have dependencies on engine2 (based on their tsconfig.json references)
  • When WASM is not built, these projects cannot be type-checked, which means some TypeScript errors could slip through

Recommendation: Consider adding a comment in the script explaining which projects depend on WASM and why they are skipped, to help future maintainers understand the trade-off.

2. Gitignore patterns (.gitignore:94-103)

Good: Comprehensive patterns for build artifacts.

Question: The pattern /website/docs/**/*.js suggests JavaScript files might be generated in the docs directory. Is this expected? If docs should only contain markdown/static content, this might indicate an unusual build configuration worth investigating.

3. Error handling consistency

Observation: The script uses a TSC_FAILED flag and continues checking all 4 projects even after one fails, which is good for developer experience (shows all errors at once). However, the temporary file handling could be slightly more robust by using separate temp files for each project check to avoid potential race conditions if the checks were ever parallelized in the future.

4. Package dependency (website/package.json)

Good: Adding rsbuild-plugin-google-analytics is the correct migration path.

Minor style note: The new dependency is listed before @emotion/styled in the diff, which suggests it might not be alphabetically sorted. This is very minor and depends on your project conventions.

Testing Recommendations

Consider adding these scenarios to your testing:

  1. Test without WASM: Verify that intentionally introduced type errors in core, website, engine2, or xmutil-js are caught
  2. Test with WASM: Verify that type errors in diagram, app, or server are caught
  3. Test the gitignore: Run yarn tsc --build and verify that git status shows no new untracked files

Code Quality Assessment

  • Correctness: Solves the stated problem
  • Best Practices: Follows TypeScript project references correctly
  • Error Handling: Good developer experience with clear error messages
  • Security: N/A
  • Performance: Efficient (only checks relevant projects)
  • Test Coverage: Consider adding integration tests for the pre-commit hook itself

Verdict

LGTM with minor suggestions

This is a solid fix that addresses a real problem. The fallback logic for missing WASM is pragmatic. The only substantive concern is documenting which projects cannot be fully type-checked without WASM, but this does not block merging since it is an improvement over the current state (no type checking at all).

The commit messages follow the project style guide perfectly, and the changes align well with the development workflow described in CLAUDE.md.

@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.31%. Comparing base (6601855) to head (fd72aef).
⚠️ Report is 33 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
+ Coverage   74.33%   76.31%   +1.97%     
==========================================
  Files          46       45       -1     
  Lines       15262    15176      -86     
==========================================
+ Hits        11345    11581     +236     
+ Misses       3917     3595     -322     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

3 participants