hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-22 21:41:37 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Update audit plans and documentation for Rojo_Ref_* attributes

Browse source 
- Revised the audit documentation to standardize the naming convention for `Rojo_Ref_*` attributes, ensuring consistency across all references.
- Enhanced the design section to clarify the handling of attribute updates during instance renames, emphasizing the need for accurate path updates.
- Adjusted the processing order in `compute_ref_properties` to prioritize `Rojo_Target_*` attributes, preventing unintended overwrites of `Rojo_Ref_*` attributes.
- Added new tests to verify the correct behavior of reference path changes and ensure robustness against edge cases.

These changes improve the clarity and reliability of the synchronization system's handling of reference attributes.
This commit is contained in:
Joe 2026-02-14 17:00:10 -06:00
parent 699fe60ba5
commit f28ee8435a
8 changed files with 44 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

16
.cursor/plans/audit_ref_two-way_sync_9e8ce7a8.plan.md
View file

@ -63,13 +63,13 @@ isProject: false
## Quality Standard
> **Round-trip identity**: Two-way sync writes a `Rojo_Ref_*` attribute to a meta/model file. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical Ref target** -- same property name, same target instance. Any deviation is a bug.
> **Round-trip identity**: Two-way sync writes a `Rojo_Ref_`* attribute to a meta/model file. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical Ref target** -- same property name, same target instance. Any deviation is a bug.
Every audit item below must be evaluated against this standard. If there is ANY code path where a Ref property can be lost, point to the wrong target, or silently become nil through a two-way-sync + rebuild cycle, flag it as **critical**.
## Code Quality Standard
Watch for duplicated logic between [ref_properties.rs](src/syncback/ref_properties.rs) (CLI syncback bulk mode) and [api.rs](src/web/api.rs) (two-way sync per-instance mode). Both compute paths and format `Rojo_Ref_*` attributes. The shared helpers in [rojo_ref.rs](src/rojo_ref.rs) should be the single source of truth.
Watch for duplicated logic between [ref_properties.rs](src/syncback/ref_properties.rs) (CLI syncback bulk mode) and [api.rs](src/web/api.rs) (two-way sync per-instance mode). Both compute paths and format `Rojo_Ref_`* attributes. The shared helpers in [rojo_ref.rs](src/rojo_ref.rs) should be the single source of truth.
---
@ -183,7 +183,7 @@ Every code path in [api.rs](src/web/api.rs) that handles Ref properties must cor
The forward-sync direction (filesystem -> server -> plugin) must correctly resolve `Rojo_Ref_*` attributes.
- [patch_compute.rs](src/snapshot/patch_compute.rs) `compute_ref_properties`:
- Correctly strips `Rojo_Ref_` prefix to get property name?
- Correctly strips `Rojo_Ref`_ prefix to get property name?
- Handles both `Variant::String` and `Variant::BinaryString` attribute values?
- Logs warning for non-string attribute types?
- `tree.get_instance_by_path(path)` returns `None` for invalid paths without crashing?
@ -219,10 +219,10 @@ The new `Rojo_Ref_*` system coexists with the legacy `Rojo_Target_*` + `Rojo_Id`
Ref attributes add a new dimension to meta file management.
- **Creation**: When the first Ref is set on an instance with no meta file -> meta file created with just `attributes: { Rojo_Ref_PropertyName: "path" }`? Does `build_meta_object` handle this (called when file doesn't exist)?
- **Merge**: When Ref attribute is added to an existing meta file with `className`, `properties`, other `attributes` -> existing content preserved? `Rojo_Ref_*` merged into attributes alongside user attributes?
- **Merge**: When Ref attribute is added to an existing meta file with `className`, `properties`, other `attributes` -> existing content preserved? `Rojo_Ref_`* merged into attributes alongside user attributes?
- **Update**: When Ref target changes -> attribute value overwritten? Old value not left alongside new?
- **Deletion**: When Ref set to nil -> attribute removed from file? If no other content remains in `attributes`, is the empty `attributes: {}` section left or cleaned up? If meta file has no remaining content at all (no className, no properties, no attributes), should the file be deleted?
- **Orphaned Rojo_Ref_***: If an instance is deleted, its meta file is deleted too (existing behavior). No orphaned `Rojo_Ref_*` attributes should remain. Verify this path.
- **Orphaned Rojo_Ref_***: If an instance is deleted, its meta file is deleted too (existing behavior). No orphaned `Rojo_Ref_`* attributes should remain. Verify this path.
---
@ -302,7 +302,7 @@ These are all updates to existing instances (not additions), so suppression is c
Verify that the two-way sync Ref output matches CLI syncback (`ref_properties.rs`) output for equivalent scenarios:
- Same `Rojo_Ref_*` attribute name format
- Same `Rojo_Ref_`* attribute name format
- Same path format (slash-separated, root excluded)
- Same JSON5 attribute placement (in `attributes` section, not `properties`)
- Same handling of nil Refs (attribute absent, not present with empty value)
@ -399,11 +399,11 @@ Evaluate existing tests against the audit areas above. For each gap, flag as mis
- **Round-trip tests**: Do integration tests verify that setting a Ref via `/api/write`, then building from filesystem, produces the same Ref?
- **Nil Ref cleanup**: Is the nil Ref -> attribute removal -> property removal chain tested end-to-end?
- **Forward-sync resolution**: Are there serve tests that verify `Rojo_Ref_*` attributes resolve to `Variant::Ref` in the tree?
- **Forward-sync resolution**: Are there serve tests that verify `Rojo_Ref_`* attributes resolve to `Variant::Ref` in the tree?
- **Path edge cases**: Instance names with `/` in them? Deep nesting? Root-level Ref targets?
- **Concurrent changes**: Multiple Ref changes in one batch?
- **Error paths**: Ref to non-existent instance? Ref on ProjectNode instance?
- **CLI syncback parity**: Does the test suite verify that two-way sync and CLI syncback produce identical `Rojo_Ref_*` output for the same input?
- **CLI syncback parity**: Does the test suite verify that two-way sync and CLI syncback produce identical `Rojo_Ref_`* output for the same input?
---

12
.cursor/plans/fix_ref_audit_issues_746c0c25.plan.md
View file

@ -66,14 +66,14 @@ Also grep for any OTHER callers of `encodePatchUpdate` that may be missing the 4
## Issue 2: Stale Rojo_Ref_* Paths After Rename (CRITICAL)
When an instance is renamed, all `Rojo_Ref_*` attributes across the tree that reference the old path must be updated to the new path.
When an instance is renamed, all `Rojo_Ref_`* attributes across the tree that reference the old path must be updated to the new path.
### Design
When `handle_tree_event` processes a rename:
1. Compute the **old path** and **new path** of the renamed instance (using `full_path_of` before and after rename)
2. Scan ALL descendants of the tree root for `Rojo_Ref_*` attributes
2. Scan ALL descendants of the tree root for `Rojo_Ref_`* attributes
3. For each attribute whose path **starts with** the old path (prefix match handles children too), replace the old prefix with the new prefix
4. Update the in-memory tree's Attributes property
5. Write the updated meta/model file to disk (with `suppress_path`)
@ -89,7 +89,7 @@ Add a new method `update_ref_paths_after_rename(&self, tree, old_path, new_path)
- For each attribute starting with `REF_PATH_ATTRIBUTE_PREFIX`, checks if the path value starts with `old_path`
- If match: replaces the prefix with `new_path`
- Updates the instance's Attributes property in the tree
- Finds the instance's meta/model file via `InstigatingSource::Path` and rewrites the `Rojo_Ref_*` attribute on disk
- Finds the instance's meta/model file via `InstigatingSource::Path` and rewrites the `Rojo_Ref_`* attribute on disk
Call this method from the rename handler, AFTER the filesystem rename is complete and the tree path has been updated. Both the directory-format rename path (line ~885) and the regular file rename path (line ~973) need it.
@ -208,13 +208,13 @@ pub fn split_ref_path(path: &str) -> Vec<String> {
## Issue 4: compute_ref_properties Priority Conflict (CRITICAL)
If both `Rojo_Ref_PrimaryPart` and `Rojo_Target_PrimaryPart` exist, `Rojo_Target_*` silently overwrites `Rojo_Ref_*` because it's processed second.
If both `Rojo_Ref_PrimaryPart` and `Rojo_Target_PrimaryPart` exist, `Rojo_Target_`* silently overwrites `Rojo_Ref_*` because it's processed second.
### Fix
**File:** [patch_compute.rs](src/snapshot/patch_compute.rs) `compute_ref_properties`
Swap the processing order: process `Rojo_Target_*` FIRST, then `Rojo_Ref_*`. Since `map.insert` overwrites, the last insert wins. Processing `Rojo_Ref_*` second means the path-based system has priority (which is correct since it's the preferred system).
Swap the processing order: process `Rojo_Target_`* FIRST, then `Rojo_Ref_*`. Since `map.insert` overwrites, the last insert wins. Processing `Rojo_Ref_*` second means the path-based system has priority (which is correct since it's the preferred system).
```rust
for (attr_name, attr_value) in attributes.iter() {
@ -301,7 +301,7 @@ Add a helper `is_ref_path_unique(tree, target_ref)` that walks ancestors checkin
**File:** [ref_properties.rs](src/syncback/ref_properties.rs) line 251
The ID-based `Rojo_Target_*` system uses inline `format!("{REF_POINTER_ATTRIBUTE_PREFIX}{}", link.name)` while the path-based system uses the shared `ref_attribute_name()` helper. Add a matching helper:
The ID-based `Rojo_Target_`* system uses inline `format!("{REF_POINTER_ATTRIBUTE_PREFIX}{}", link.name)` while the path-based system uses the shared `ref_attribute_name()` helper. Add a matching helper:
```rust
// In rojo_ref.rs: