fix(): use params entity registry for update_persistent_entities#1898
fix(): use params entity registry for update_persistent_entities#1898benflexcompute wants to merge 1 commit intomainfrom
Conversation
…egacy workflow The legacy workflow in set_up_params_for_uploading had two issues causing entity modifications (e.g. volume zone center/axis) to not reflect in the uploaded simulation.json: 1. entity_info deep copy was discarded after update_persistent_entities, then a fresh copy was used downstream — losing all updates. 2. root_asset.internal_registry was used as the entity source, which does not contain modifications when the user operates on a different asset object than project._root_asset. Fix: obtain one deep copy of entity_info upfront and build a deduplicated entity registry from params.used_entity_registry (which contains the user's actual modified entities from stored_entities in models). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: 8873b37fe7
ℹ️ 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".
| key = (entity_type, entity.private_attribute_id) | ||
| if key not in seen_keys: |
There was a problem hiding this comment.
This deduplication key uses entity.private_attribute_id, but private_attribute_id is optional and defaults to None for many persistent entities; when that happens, all entities of the same type collapse into one entry. In the legacy upload path, update_persistent_entities() then resolves by id and can reuse that single entity across unrelated faces/edges/zones, corrupting project_entity_info (names/colors/metadata) whenever params contain multiple same-type entities with id=None.
Useful? React with 👍 / 👎.
| if key not in seen_keys: | ||
| registry.register(entity) | ||
| seen_keys.add(key) |
There was a problem hiding this comment.
When multiple objects share the same (entity_type, private_attribute_id), this keeps the first occurrence, but traversal order in params.used_entity_registry is structural rather than edit-recency. If a stale copy is encountered before an edited copy (e.g., same asset referenced in multiple models/outputs), the stale object wins and user modifications are dropped before update_persistent_entities runs.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| key = (entity_type, entity.private_attribute_id) | ||
| if key not in seen_keys: | ||
| registry.register(entity) | ||
| seen_keys.add(key) |
There was a problem hiding this comment.
Dedup key with None ID silently drops distinct entities
Low Severity
The dedup key (entity_type, entity.private_attribute_id) in _build_deduplicated_entity_registry_from_params collapses all entities of the same type that have private_attribute_id=None into a single entry. If params contains multiple distinct entities of the same type where private_attribute_id is None (e.g., multiple surfaces without assigned IDs), only the first is registered and the rest are silently dropped. While find_by_asset_id currently can't match None-id entities anyway (it validates entity_id is a str), this makes the function's behavior surprising and fragile if its output is ever reused for other purposes.
1 participant
Summary
set_up_params_for_uploadingwhere volume zone modifications (center/axis) made on a separate VolumeMesh object were not reflected in the uploaded simulation.jsonupdate_persistent_entities, androot_asset.internal_registrymissed modifications from different asset referencesparams.used_entity_registryinstead, which always contains the user's actual modified entities fromstored_entitiesTest plan
test_root_asset_entity_change_reflectionto reproduce the bug (separate VolumeMesh object modifying zone center/axis)test_project.pypasstest_persistent_entity_info_update_volume_meshandtest_persistent_entity_info_update_geometrypass🤖 Generated with Claude Code
Note
Medium Risk
Changes legacy
set_up_params_for_uploadingbehavior for how persistent entities are updated before upload, which can affect what entity metadata (e.g., volume zone center/axis) is serialized intosimulation.json. The change is localized and covered by an added regression test, but impacts core pre-upload parameter preparation.Overview
Fixes the legacy (non-
DraftContext) upload path so persistent entity updates come fromparams.used_entity_registryrather than the root asset’s registry, ensuring user edits made via separate asset instances are reflected in the uploadedsimulation.json.Adds
_build_deduplicated_entity_registry_from_params()to de-dup entities by(entity_type, private_attribute_id)before callingupdate_persistent_entities, updates VM mocksimulationJsonto include zone IDs, and extendstest_root_asset_entity_change_reflectionto cover volume-zonecenter/axispropagation.Written by Cursor Bugbot for commit 8873b37. This will update automatically on new commits. Configure here.