engine: replace protobuf patch types with native Rust types, add module support#240
engine: replace protobuf patch types with native Rust types, add module support#240
Conversation
…le support The apply_patch API previously converted JSON input to protobuf (project_io::ProjectPatch) as an intermediate format before applying operations to the datamodel. This was a leftover from when the API was protobuf-based. This change defines native Rust patch types (ProjectPatch, ModelPatch, ProjectOperation, ModelOperation) that use datamodel types directly, eliminating the protobuf round-trip. The JSON conversion in libsimlin now goes directly from JSON -> native patch types -> datamodel, which is simpler and more maintainable. Additionally, this adds: - AddModel project operation for creating new models within a patch transaction, enabling atomic "add model + add module referencing it" - Comprehensive test coverage for module upsert, delete, replace, module references, and combined AddModel+UpsertModule operations - Test for patch rollback behavior on error - Fix pre-existing TS type error in core/datamodel.ts (List<string>) https://claude.ai/code/session_011dCiV2VEYEPkUekXvBayyh
Code Review - PR #240I've reviewed the changes in this PR that replaces protobuf patch types with native Rust types and adds module support. Overall, this is a solid refactoring that simplifies the codebase. Here are my findings: Positive AspectsArchitecture Improvements:
Code Quality:
Testing:
Issues and Concerns1. Missing Clone Derivation Context (Minor) In
2. Error Handling Simplification Loses Information In // Before
let patch_proto = match convert_json_project_patch(json_patch) {
Ok(patch) => patch,
Err(err) => {
store_ffi_error(out_error, err);
return;
}
};
// After
let patch = convert_json_project_patch(json_patch);The new signature returns 3. Canonicalization Happens at Application Time Lines 418-435 in
Consider: Should canonicalization happen during JSON→Rust conversion instead? 4. Test Coverage Gap While test coverage is excellent, I notice:
5. Documentation The new public types (
6. TypeScript Type Fix is Good The fix in Minor IssuesCode Style:
Consistency:
Security ConsiderationsNo security concerns identified. The patch application properly:
Performance Considerations
Recommendations
SummaryThis is a well-executed refactoring that meaningfully improves the codebase. The elimination of protobuf as an intermediate format is the right choice, and the new native types are cleaner and more maintainable. The test coverage is excellent, and the atomic patch application with rollback is properly implemented. The concerns raised are minor and mostly about future maintainability rather than current correctness. I recommend merging this PR with consideration for adding documentation to the new public APIs. Verdict: Approve with minor suggestions ✅ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #240 +/- ##
==========================================
- Coverage 76.53% 76.47% -0.06%
==========================================
Files 80 80
Lines 21828 21763 -65
==========================================
- Hits 16705 16643 -62
+ Misses 5123 5120 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 9265e9bb26
ℹ️ 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".
src/simlin-engine/src/patch.rs
Outdated
| fn apply_add_model(project: &mut datamodel::Project, name: &str) -> Result<()> { | ||
| let canonical_name = canonicalize(name); | ||
| // Check if a model with this name already exists |
There was a problem hiding this comment.
apply_add_model canonicalizes the provided model name and stores the canonicalized string, but Project::get_model(_mut) matches model names literally (no canonicalization). This means a patch that adds a model like "Customer Growth" will create a model named customer_growth, and any subsequent operations (including in the same patch) that refer to the original display name will fail with BadModelName. It also silently changes the user-visible model name. Consider storing the name as provided (like other code paths do) or canonicalizing lookups consistently.
Useful? React with 👍 / 👎.
…e conversion Three issues addressed: 1. apply_add_model was the only code path that canonicalized model names before storing. XMILE import, JSON import, and the C FFI simlin_project_add_model all store names as-provided. Since get_model/get_model_mut use literal string equality, canonicalizing on add broke same-patch references using the original display name (e.g. "Customer Growth" would become "customer_growth" and then a ModelPatch targeting "Customer Growth" would fail with BadModelName). Now stores names as-provided, matching all other code paths. 2. apply_patch now takes ProjectPatch by value instead of by reference. This eliminates all .clone() calls on the operation payloads (Stock, Flow, Aux, Module, View, SimSpecs, Source) since we can move data directly out of the consumed patch. The Clone derive is removed from the patch types. 3. convert_json_project_patch in libsimlin returns Result again. While the current conversion arms are all infallible, returning Result preserves the ability to add validation without changing the call site, and matches the established pattern in the FFI layer. https://claude.ai/code/session_011dCiV2VEYEPkUekXvBayyh
Code ReviewThis PR refactors the patch API to use native Rust types instead of protobuf intermediates, and adds module support including an AddModel operation. The changes are well-structured and improve code maintainability. Here's my detailed review: ✅ Strengths1. Architecture Improvement
2. Test Coverage
3. Code Simplification
4. Fixes Critical Issues
🔍 Observations & Minor Concerns1. API Signature Change fn convert_json_project_patch(
patch: JsonProjectPatch,
) -> std::result::Result<engine::ProjectPatch, FfiError>However, all current conversion arms are infallible. This is fine as stated in the commit message, but consider:
2. Missing SetSource in JsonProjectOperation pub enum ProjectOperation {
SetSimSpecs(datamodel::SimSpecs),
SetSource(datamodel::Source), // <-- This one
AddModel { name: String },
}But enum JsonProjectOperation {
SetSimSpecs { sim_specs: ... },
AddModel { name: String },
// Missing SetSource
}Is this intentional? If 3. Error Messages Some(format!("model '{}' already exists", name))Consider including the project name in error messages to help with debugging in multi-project scenarios. 4. Test Pattern Consistency
📋 Code Quality DetailsType Safety: ✅ Excellent use of Rust's type system to ensure correctness Memory Management: ✅ The move semantics ( Error Handling: ✅ Proper use of Documentation: 🔐 Security Considerations✅ No security concerns identified:
🎯 Performance Considerations✅ Performance improved:
🧪 Testing RecommendationsWhile test coverage is excellent, consider adding:
📝 Documentation Suggestions
SummaryOverall Assessment: ✅ Approve with minor suggestions This is a high-quality refactoring that:
The code follows Rust best practices and the project's conventions. The minor observations above are suggestions for improvement rather than blockers. Recommended Action: Merge after addressing the Great work! 🚀 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
The apply_patch API previously converted JSON input to protobuf
(project_io::ProjectPatch) as an intermediate format before applying
operations to the datamodel. This was a leftover from when the API was
protobuf-based.
This change defines native Rust patch types (ProjectPatch, ModelPatch,
ProjectOperation, ModelOperation) that use datamodel types directly,
eliminating the protobuf round-trip. The JSON conversion in libsimlin
now goes directly from JSON -> native patch types -> datamodel, which
is simpler and more maintainable.
Additionally, this adds:
transaction, enabling atomic "add model + add module referencing it"
module references, and combined AddModel+UpsertModule operations
https://claude.ai/code/session_011dCiV2VEYEPkUekXvBayyh