engine2: expose Vensim MDL file support

[bpowers]

Add Project.openVensim() and Project.hasVensimSupport() static methods
to the engine2 public API. These methods wrap the underlying libsimlin
FFI functions and provide proper error handling when Vensim support is
not available (e.g., when WASM is built without the vensim feature).

The hasVensimSupport() function is also exported directly from the
module for cases where users want to check availability without using
the Project class.

Updated NewProject.tsx to use direct Vensim import when available,
falling back to the xmutil MDL-to-XMILE conversion when not. This
makes the code future-proof for when the browser WASM build might
include Vensim support.

Added comprehensive unit tests for the new Vensim APIs that properly
handle both cases: when Vensim support is available and when it is not.

[claude]

Code Review: Vensim MDL Support in engine2 API

Summary

This PR adds Vensim MDL file support to the engine2 public API by exposing Project.openVensim() and Project.hasVensimSupport() methods. The implementation is well-structured with proper feature detection and comprehensive test coverage.

Strengths

1. Excellent Feature Detection Pattern

The implementation correctly handles optional Vensim support through runtime feature detection:

• hasVensimSupport() checks for the WASM export before attempting to use it
• Clear error messaging when Vensim support is unavailable
• Both static method and direct export for flexibility

2. Comprehensive Test Coverage

The test suite in api.test.ts is exemplary:

• Tests both availability scenarios (with and without Vensim support)
• Validates string and Uint8Array input formats
• Includes functional simulation tests, not just API existence checks
• Uses proper conditional logic to skip tests when Vensim is unavailable
• Fails loudly if required test files are missing (lines 50-52)

3. Graceful Degradation in NewProject.tsx

The UI code properly falls back to xmutil conversion when direct Vensim support is unavailable, making it future-proof.

4. Consistent API Design

The openVensim() method mirrors the existing open() method's signature and behavior, maintaining API consistency.

Issues & Recommendations

Critical Issues

None identified.

Minor Issues

1. Inconsistent Variable Declaration Style (src/app/NewProject.tsx:145)

Location: src/app/NewProject.tsx:145

Changed from let contents to const contents, but this is inconsistent with the existing let logs on line 146. While const is preferred, this should be consistent throughout the function.

Recommendation: Either change both to const or keep both as let. Given that logs is reassigned on line 159, keeping let for both might be more consistent with the existing code style.

2. Test File Path Assumption

Location: src/engine2/tests/api.test.ts:49-51

The test throws an error if the teacup.mdl file doesn't exist. While this follows the repository's testing philosophy of "failing loudly," it might be helpful to verify this file exists in the test setup or CI.

Recommendation: Verify that test/test-models/samples/teacup/teacup.mdl is committed to the repository and included in test fixtures. ✓ (Confirmed: file exists)

3. Error Message Specificity

Location: src/engine2/src/internal/import-export.ts:73-77

The error message is clear, but could mention how users might enable Vensim support (rebuild instructions).

Current:

'simlin_project_open_vensim is not available: libsimlin was built without Vensim support. ' +
  'Rebuild with the "vensim" feature enabled to import MDL files.'

Suggestion: This is actually quite good! No change needed.

Code Quality Observations

Positive Patterns:

1. Memory Safety: Proper cleanup in finally blocks (import-export.ts:97-100)
2. Type Safety: Proper type narrowing with runtime checks (import-export.ts:72)
3. Documentation: Clear JSDoc comments explaining feature availability
4. Error Handling: Consistent error propagation using SimlinError

Follows Repository Conventions:

• ✓ Uses proper TypeScript types, avoids any
• ✓ Clear commit message following the style guide
• ✓ Tests validate actual behavior, not just API existence
• ✓ No unnecessary documentation files created
• ✓ Prefers editing existing files over creating new ones

Performance Considerations

No performance concerns identified. The feature detection is a simple function property check, which is negligible overhead.

Security Considerations

No security issues identified. The FFI boundary properly validates null pointers and buffer lengths in the Rust implementation.

Architecture Notes

The implementation correctly:

1. Mirrors the existing simlin_project_open_xmile pattern
2. Guards against missing WASM exports at the TypeScript layer
3. Provides both high-level (Project.openVensim) and low-level (simlin_project_open_vensim) interfaces
4. Maintains separation of concerns (FFI layer → Project wrapper → UI)

Verdict

APPROVED ✓

This is high-quality code that follows best practices and repository conventions. The implementation is clean, well-tested, and properly handles edge cases. The only suggestion is the minor variable declaration consistency in NewProject.tsx, which is cosmetic.

Suggested Follow-up

Consider documenting the Vensim feature flag in the build documentation so users know how to enable it for their WASM builds (outside scope of this PR).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74192a2b95

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

[chatgpt-codex-connector]

Guard hasVensimSupport before WASM init

The newly exported hasVensimSupport is a sync re-export of the internal helper, which calls getExports() and throws when WASM hasn’t been initialized yet. That means a consumer who calls this “availability check” before ready() (e.g., to decide whether to use MDL import or a fallback) will get a runtime exception instead of a boolean. Consider either making this export async and calling ensureInitialized, or documenting/renaming it so callers know they must initialize first.

Useful? React with 👍 / 👎.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.14%. Comparing base (9bd75d4) to head (1dc8ad1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
- Coverage   76.16%   76.14%   -0.03%     
==========================================
  Files          45       45              
  Lines       15256    15256              
==========================================
- Hits        11620    11616       -4     
- Misses       3636     3640       +4     

☔ 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.

[claude]

Code Review

This PR adds Vensim MDL file support to the engine2 public API. Overall, the implementation is solid and follows good practices. Here are my findings:

✅ Strengths

1. Excellent error handling: The implementation properly handles the case where Vensim support is not available (when WASM is built without the vensim feature), throwing clear error messages.

2. Consistent API design: The Project.openVensim() method mirrors the existing Project.open() pattern, accepting both string and Uint8Array inputs, which provides a consistent developer experience.

3. Comprehensive test coverage: The test suite covers multiple scenarios:

   • Checking availability via hasVensimSupport()
   • Handling missing support gracefully
   • Loading MDL files when support is available
   • Accepting both string and Uint8Array inputs
   • Running simulations on loaded models

4. Good defensive programming: The simlin_project_open_vensim() function in import-export.ts checks if the function exists before calling it, preventing runtime errors.

5. Future-proof implementation: NewProject.tsx now tries direct Vensim import first, falling back to xmutil conversion when unavailable, which will automatically benefit from native support when available.

6. Memory management: Proper cleanup with try/finally blocks ensuring WASM memory is freed.

⚠️ Issues Found

1. Missing export in index.ts (mentioned in PR description but not implemented)

The PR description states:

The hasVensimSupport() function is also exported directly from the module for cases where users want to check availability without using the Project class.

However, src/engine2/src/index.ts does NOT export hasVensimSupport. This means users cannot do:

import { hasVensimSupport } from '@system-dynamics/engine2';

They can only use Project.hasVensimSupport(). Either:

• Add the export to index.ts, OR
• Update the PR description to remove this claim

Recommendation: Add this line to src/engine2/src/index.ts:

export { hasVensimSupport } from './internal/import-export';

2. Inconsistent access modifier

In src/engine2/src/project.ts:110, fromVensim() is marked as private, while the equivalent fromXmile() (line 88) is also private. However, the comment on fromVensim doesn't explain why it's private when there's a public openVensim() wrapper. This is fine, but consider adding a brief comment explaining the pattern (internal factory method with public async wrapper).

💡 Suggestions

1. Test file existence check: The test helper loadTestMdl() throws an error if the file doesn't exist, which is good. However, you might consider using test.skip() or similar to skip Vensim tests when the test file is unavailable, rather than failing the entire test suite. (This is minor - current approach is acceptable for committed test files.)

2. Error message clarity: In NewProject.tsx line 161, when the conversion fails, consider including information about whether the fallback path was taken:

throw new Error('Vensim converter (using fallback conversion): ' + (logs || 'unknown error'));

3. Type safety: Consider adding a type guard or assertion that engine2Project is defined before use (lines 170+), though TypeScript's control flow analysis should handle this.

🔒 Security

No security concerns identified. The implementation properly validates inputs and handles errors.

⚡ Performance

No performance concerns. The feature check (hasVensimSupport()) is a simple type check that won't impact performance.

📋 Commit Message

The commit message follows the project's style guide well:

• Proper prefix: "engine2:"
• Concise summary
• Detailed body explaining the "why"
• No emoji ✓

Summary

This is a well-implemented feature with only one issue: the missing export mentioned in the PR description. Fix that and this is ready to merge.

Action Required: Add hasVensimSupport export to index.ts or update the PR description.