diff --git a/.cursor/plans/fix_audit_pass_2_c72b80c5.plan.md b/.cursor/plans/fix_audit_pass_2_c72b80c5.plan.md new file mode 100644 index 00000000..49acf5ad --- /dev/null +++ b/.cursor/plans/fix_audit_pass_2_c72b80c5.plan.md @@ -0,0 +1,334 @@ +--- +name: Fix audit pass 2 +overview: Fix 1 critical bug, 4 correctness issues, 3 cleanup items, and add 9 missing tests identified by the slugify branch audit pass 2. +todos: + - id: cc2-refactor + content: "CRITICAL: Replace inline removal in handle_api_write with new remove_instance_at_path method. Delete dead syncback_removed_instance." + status: completed + - id: preseed-fix + content: Fix pre-seeding in dir.rs and project.rs to derive dedup keys from inst.metadata().relevant_paths + strip_middleware_extension, not slugified instance names. + status: completed + - id: rename-dedup + content: "Add dedup logic to change_processor.rs rename handler: on collision, scan parent dir, build taken_names, run deduplicate_name." + status: completed + - id: dir-sort + content: Add alphabetical sort to dir.rs child iteration to match project.rs for deterministic dedup ordering. + status: completed + - id: csv-instigating + content: Add instigating_source preservation to csv.rs snapshot_csv_init, matching lua.rs pattern. + status: completed + - id: over-suppression + content: "Fix over-suppression in remove_meta_name_field: add suppress_path_remove to ChangeProcessor, swap FileDeleted branch to (1,0) counts." + status: completed + - id: comment-fix + content: Update misleading comment in change_processor.rs:608-609 to reference remove_instance_at_path. + status: completed + - id: build-tests + content: "Add 4 build test fixtures: meta_name_override, model_json_name_override, dedup_suffix_with_meta, init_meta_name_override." + status: completed + - id: syncback-tests + content: "Add 3 syncback test scenarios: forbidden chars, collision, idempotency." + status: completed + - id: twoway-tests + content: "Add 2 two-way sync tests: rename clean->slugified, rename slugified->clean." + status: completed +isProject: false +--- + +# Fix Audit Pass 2 Findings + +## Standards (from audit plan) + +> **Round-trip identity**: Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical instance tree** -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug. + +> **Code quality**: DRY -- no duplicated slugify/dedup/meta patterns. Shared helpers over copy-paste. If the same "compute meta path, check if name needs slugify, upsert/remove name field" sequence appears in 3+ places, that's a refactor candidate. + +> **Legacy %ENCODING%**: Intentionally unsupported. No backward compat for old `%NAME%` patterns. + +> **When working on two-way sync:** Plugin-based sync must produce **exactly the same filesystem output** that `rojo syncback` would give for the same input. Byte-for-byte identical files, identical directory structures, identical naming. Any divergence is a bug until proven otherwise. + +--- + +## CRITICAL 1: CC2 init-file removal broken in active code path + +**Problem:** The inline removal in `handle_api_write` ([src/web/api.rs](src/web/api.rs) lines 363-451) uses `p.is_dir()` to decide how to delete. For init-file scripts (e.g. `src/MyModule/init.luau`), `instigating_source` is the init file path so `p.is_dir()` = false. Only the init file gets deleted; the parent directory and siblings survive. The correct fix exists in the dead `syncback_removed_instance` (lines 1478-1555) but is never called. + +**Fix:** Replace the inline removal with a new `remove_instance_at_path` method and delete `syncback_removed_instance`. + +- Extract the filesystem operation logic from `syncback_removed_instance` (lines 1478-1555) into a new method `remove_instance_at_path(&self, path: &Path) -> anyhow::Result<()>` on `ApiService`. This handles: directory removal, init-file detection (parent dir removal + grandparent meta cleanup), regular file removal + adjacent meta cleanup via `adjacent_meta_path()`. +- Keep Phase 1 (path gathering under lock, lines 357-385) as-is -- it resolves `instigating_source -> PathBuf` while holding the tree lock. +- Replace the inline Phase 2 (lines 388-451) with a loop calling `remove_instance_at_path(path)` for each gathered path. +- Delete the `#[allow(dead_code)] syncback_removed_instance` function (lines 1428-1557) since its logic now lives in `remove_instance_at_path`. +- Suppress filesystem events inside `remove_instance_at_path` using the existing `suppress_path_remove` method. + +--- + +## FIX 2: Pre-seeding uses slugified names instead of filesystem dedup keys + +**Problem:** In [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs) lines 196-204 and [src/snapshot_middleware/project.rs](src/snapshot_middleware/project.rs) lines 670-678, pre-seeding inserts `slugify_name(inst.name()).to_lowercase()`. But if an old instance already has a tilde-suffixed file (e.g. `A_B~1.luau` from prior dedup), only `"a_b"` is seeded, not `"a_b~1"`. A new-only instance processed before the old can claim `"a_b~1"`, colliding with the existing file. + +**Fix:** Derive dedup keys from the old instance's actual filesystem path, matching what `name_for_inst` (line 32-42 of [src/syncback/file_names.rs](src/syncback/file_names.rs)) does for old instances. The old instance metadata has `relevant_paths` and `middleware`: + +```rust +// In both dir.rs and project.rs pre-seeding loops: +for inst in old_child_map.values() { + if let Some(path) = inst.metadata().relevant_paths.first() { + if let Some(filename) = path.file_name().and_then(|f| f.to_str()) { + let middleware = inst.metadata().middleware.unwrap_or(Middleware::Dir); + let dedup_key = strip_middleware_extension(filename, middleware).to_lowercase(); + taken_names.insert(dedup_key); + } + } else { + // Fallback (shouldn't happen for old instances) + let name = inst.name(); + let slug = if name_needs_slugify(name) { + slugify_name(name).to_lowercase() + } else { + name.to_lowercase() + }; + taken_names.insert(slug); + } +} +``` + +This uses `inst.metadata().relevant_paths.first()` (actual filesystem path) + `inst.metadata().middleware` + `strip_middleware_extension()` -- the same pipeline as `name_for_inst`'s old-inst path. Both `dir.rs` and `project.rs` need the same fix. + +--- + +## FIX 3: Rename collision -- no dedup against siblings + +**Problem:** [src/change_processor.rs](src/change_processor.rs) rename handler slugifies the new name but does NOT call `deduplicate_name()`. If the new slug collides with an existing sibling file, `fs::rename` silently overwrites on Unix (data loss) or fails on Windows. + +**Fix:** After computing the new path, before calling `fs::rename`, check if the target already exists (and isn't the source path). If collision detected, scan the parent directory to build a `taken_names` set, then run `deduplicate_name()`: + +- For both the **init-file path** (dir rename, ~~line 697) and **regular file path** (~~line 810): + 1. Compute `new_path` from `slugified_new_name` + 2. If `new_path != old_path && new_path.exists()`: + - Read the parent directory entries, extract bare slugs (using `file_stem()` + `strip_script_suffix()` for files, `file_name()` for dirs), lowercase, collect into `HashSet` + - Remove the old path's own slug from the set (it's about to be freed) + - Run `deduplicate_name(&slugified_new_name, &taken_names)` + - Recompute `new_path` with the deduped name + 3. The resulting `new_path` is guaranteed unique + +This scan only happens on actual collision (the `new_path.exists()` guard), so the common case has zero overhead. + +--- + +## FIX 4: `dir.rs` child ordering not sorted + +**Problem:** [src/snapshot_middleware/dir.rs](src/snapshot_middleware/dir.rs) processes children in Roblox DOM order (`new_inst.children()`). Unlike `project.rs` which sorts alphabetically (CC1 fix at line 685-686), `dir.rs` doesn't sort. Dedup suffix assignment (`~1` vs base) can differ across serializations. + +**Fix:** Collect child refs with their names, sort alphabetically, then iterate in sorted order. Apply to the main child iteration loop starting at line 207: + +```rust +// Collect children with names for deterministic ordering +let mut sorted_children: Vec<_> = new_inst.children().iter() + .map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone())) + .collect(); +sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b)); + +for (new_child_ref, _) in &sorted_children { + let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); + // ... existing loop body ... +} +``` + +Also apply the same sort to the new-only children loop starting at ~line 295 (the `for new_child_ref in new_inst.children()` after the matched-children loop). + +--- + +## FIX 5: `csv.rs` `snapshot_csv_init` doesn't preserve `instigating_source` + +**Problem:** [src/snapshot_middleware/csv.rs](src/snapshot_middleware/csv.rs) line 81 does `init_snapshot.metadata = dir_snapshot.metadata;` without preserving the CSV file's `instigating_source`. Two-way sync writes target the directory instead of the file. Compare with `lua.rs` lines 114-123 which correctly saves and restores. + +**Fix:** Add the same preservation pattern from `lua.rs`: + +```rust +// Before copying dir metadata, save the init script's instigating_source +let script_instigating_source = init_snapshot.metadata.instigating_source.take(); + +init_snapshot.children = dir_snapshot.children; +init_snapshot.metadata = dir_snapshot.metadata; + +// Restore so two-way sync writes to the CSV file, not the directory +init_snapshot.metadata.instigating_source = script_instigating_source; +``` + +--- + +## FIX 6: Over-suppression in `remove_meta_name_field` + +**Problem:** In [src/change_processor.rs](src/change_processor.rs) lines 245-271, when `RemoveNameOutcome::FileDeleted`, the code calls `suppress_path` (create_write_count += 1) then `suppress_path_any` (remove_count += 1, create_write_count += 1), yielding `(1, 2)`. The correct counts should be `(1, 0)` -- one Remove event, zero Write events. + +**Fix:** + +1. Add a `suppress_path_remove` method to `ChangeProcessor` (matching the one in `api.rs:253-257`): `entry.0 += 1` (only remove_count). +2. Rewrite the `FileDeleted` branch: + +```rust +Ok(RemoveNameOutcome::FileDeleted) => { + // Was a delete, not a write. Swap: undo Write, add Remove. + self.unsuppress_path(meta_path); // undo (0, 1) -> (0, 0) + self.suppress_path_remove(meta_path); // add (1, 0) +} +``` + +--- + +## FIX 7: Update misleading comment in `change_processor.rs` + +**Problem:** [src/change_processor.rs](src/change_processor.rs) lines 608-609 reference `syncback_removed_instance` as if it's the active removal path. After the CC2 refactor, it will reference the now-deleted function. + +**Fix:** Update the comment to accurately describe the new path: + +```rust +// NOTE: We do NOT delete files from disk here. The API handler +// (handle_api_write → remove_instance_at_path) already deleted +// the files before sending this PatchSet. ... +``` + +--- + +## Tests: 9 new test scenarios + +### Build tests (4 new fixtures in `rojo-test/build-tests/`) + +1. `**meta_name_override**` -- `Hey_Bro.luau` + `Hey_Bro.meta.json5` with `{"name": "Hey/Bro"}`. Assert instance name = `"Hey/Bro"`, not `"Hey_Bro"`. +2. `**model_json_name_override**` -- `Hey_Bro.model.json5` with `{"name": "Hey/Bro", "className": "Configuration"}`. Assert instance name = `"Hey/Bro"`. +3. `**dedup_suffix_with_meta**` -- `Foo.luau` + `Foo~1.luau` + `Foo~1.meta.json5` with `{"name": "Foo"}`. Assert two instances: one named from each file's meta. +4. `**init_meta_name_override**` -- `MyFolder/init.luau` + `MyFolder/init.meta.json5` with `{"name": "Real:Name"}`. Assert directory instance name = `"Real:Name"`. + +Each needs a `default.project.json5` pointing `$path` at the source dir, and a snapshot file (generated via `cargo test` + `cargo insta review`). + +### Syncback tests (3 new scenarios) + +1. **Forbidden chars** -- Build fixture with instances named `"Hey/Bro"` and `"Key:Script"`. Run syncback. Assert slugified filenames + `.meta.json5` with correct `name` fields. +2. **Collision** -- Two sibling instances `"A/B"` and `"A:B"` (both slug to `A_B`). Assert one gets `A_B.luau`, other gets `A_B~1.luau`, both with correct meta. +3. **Idempotency** -- Run syncback twice on same tree. Assert zero file changes on second run. + +### Two-way sync tests (2 new scenarios in `tests/tests/two_way_sync.rs`) + +1. **Rename clean to slugified** -- Rename `Normal` to `Hey/Bro`. Assert file becomes `Hey_Bro.luau` + `Hey_Bro.meta.json5` with `{"name": "Hey/Bro"}`. +2. **Rename slugified to clean** -- Rename `What?Module` to `CleanName`. Assert file becomes `CleanName.luau`, meta `name` field removed (or meta file deleted if no other fields). + +--- + +## Implementation Notes + +### Files modified + + +| File | What changed | +| ------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `src/web/api.rs` | **CC2 fix**: Replaced inline Phase 2 removal (lines 388-451) with calls to new `remove_instance_at_path(&self, path: &Path) -> bool`. Simplified Phase 1 to gather `Vec<(Ref, Option)>` (no `is_dir` needed). Deleted dead `syncback_removed_instance` function. The new method handles: directory removal, init-file detection (parent dir removal + grandparent meta), regular file removal + `adjacent_meta_path()`. | +| `src/snapshot_middleware/dir.rs` | **Pre-seeding fix**: Changed to derive dedup keys from `inst.metadata().relevant_paths.first()` + `strip_middleware_extension()` instead of slugifying instance names. Added `strip_middleware_extension` and `Middleware` to imports. **Dir sort**: Added alphabetical sorting to both child iteration loops (old+new children loop, and new-only children loop). | +| `src/snapshot_middleware/project.rs` | **Pre-seeding fix**: Same filesystem-based dedup key derivation as dir.rs. Added `strip_middleware_extension` to imports. | +| `src/change_processor.rs` | **Rename dedup (file path only)**: Added collision detection for regular file renames -- scans parent dir, builds `taken_names`, runs `deduplicate_name()`. Uses `deduped_new_name` for meta file naming. **Over-suppression fix**: Added `suppress_path_remove` method, rewrote `FileDeleted` branch to swap from Write to Remove suppression `(1, 0)`. **Comment fix**: Updated line 621 to reference `remove_instance_at_path`. Added `deduplicate_name`, `strip_script_suffix` to imports. | +| `src/snapshot_middleware/csv.rs` | **CSV instigating_source**: Added `instigating_source` preservation before copying `dir_snapshot.metadata`, matching `lua.rs` pattern. | +| `src/syncback/mod.rs` | Added `strip_script_suffix` to the re-exports from `file_names`. | + + +### Files created + + +| File | Purpose | +| ----------------------------------------------------- | ------------------------------------------------------------------------------------------------ | +| `rojo-test/build-tests/meta_name_override/` | Build fixture: `.meta.json5` `name` field overrides instance name to `Hey/Bro`. | +| `rojo-test/build-tests/model_json_name_override/` | Build fixture: `.model.json5` `name` field overrides instance name to `Hey/Bro`. | +| `rojo-test/build-tests/dedup_suffix_with_meta/` | Build fixture: `Foo.luau` + `Foo~1.luau` each with meta name overrides (`A/B` and `A_B`). | +| `rojo-test/build-tests/init_meta_name_override/` | Build fixture: `init.meta.json5` inside folder overrides directory instance name to `Real:Name`. | +| `rojo-test/build-test-snapshots/` (4 new .snap files) | Accepted insta snapshots for the 4 build tests above. | + + +### Snapshots updated + + +| Snapshot | Change | Reason | +| -------------------------- | ---------------------------------------------------------- | ---------------------------------------------------------------------------------------------- | +| `csv_init.snap` | `instigating_source: Path: /root` → `Path: /root/init.csv` | CSV instigating_source fix -- now correctly points to the init file, matching lua.rs behavior. | +| `csv_init_with_meta.snap` | Same `instigating_source` change. | Same fix. | +| 4 new build-test snapshots | Created from scratch, accepted via `cargo insta accept`. | New build test fixtures. | + + +### Key design decision: directory rename dedup NOT applied + +**Finding during implementation:** The rename dedup logic was initially added to both the **init-file/directory rename** path and the **regular file rename** path. However, this broke two existing tests: + +- `failed_directory_rename_no_stale_suppression` +- `failed_directory_rename_then_successful_rename` + +These tests intentionally block a directory rename with a non-empty directory and assert the rename fails gracefully. With dedup, the rename silently succeeded by appending `~1`, which is confusing user behavior (user asked for name X, got X~1 instead). + +**Resolution:** Dedup is only applied to the **regular file rename path** (where Unix `fs::rename` can silently overwrite). Directory renames are left without dedup because: + +1. `fs::rename` to a non-empty directory fails safely on **all** platforms (Windows and Unix) +2. `fs::rename` to an empty directory on Unix replaces it, but this is a much rarer edge case +3. Silently deduplicating a directory name is more confusing than failing with an error + +### Test results + +- **649 lib tests pass** (3 new in `file_names.rs`: forbidden chars, collision, idempotency) +- **312 integration tests pass** (4 new build tests, 2 new two-way sync tests) +- **0 failures, 0 clippy warnings** + +### Audit pass 2 findings -- verification status + + +| Original finding | Status | Notes | +| ----------------------------- | ------------------------ | ------------------------------------------------------------------------------------------------------- | +| C1 (meta gating) | Verified FIXED in pass 1 | `effective_dir_path`/`effective_meta_base` pattern confirmed correct | +| C2 (removal meta path) | Verified FIXED in pass 1 | Inline code uses `path.file_stem()`, now replaced by `remove_instance_at_path` + `adjacent_meta_path()` | +| C3 (deny_unknown_fields) | Verified FIXED in pass 1 | `SyncbackRules` no longer has `deny_unknown_fields` | +| CC1 (project.rs determinism) | Verified FIXED in pass 1 | Alphabetical sort at line 685-686 confirmed | +| **CC2 (init-file removal)** | **FIXED in pass 2** | Was only in dead code; now wired into active `handle_api_write` via `remove_instance_at_path` | +| DR1 (shared meta helper) | Verified FIXED in pass 1 | `syncback::meta` module with `upsert_meta_name`/`remove_meta_name` | +| DR2 (meta path helper) | Verified FIXED in pass 1 | `adjacent_meta_path()` and `strip_script_suffix()` in `file_names.rs` | +| **Pre-seeding bug** | **FIXED in pass 2** | Both `dir.rs` and `project.rs` now derive from `relevant_paths` + `strip_middleware_extension` | +| **Rename collision** | **FIXED in pass 2** | File renames dedup; directory renames left as-is (safe on all platforms) | +| **dir.rs ordering** | **FIXED in pass 2** | Both child loops now sort alphabetically, matching `project.rs` | +| **csv.rs instigating_source** | **FIXED in pass 2** | Preservation pattern added, matching `lua.rs` | +| **Over-suppression** | **FIXED in pass 2** | `suppress_path_remove` added, `FileDeleted` branch now `(1, 0)` | + + +### Forward sync audit results (pass 2) + +All middleware correctly resolves instance names: + +- `meta_file.rs`: Both `AdjacentMetadata` and `DirectoryMetadata` have `name: Option`, `apply_name` sets `specified_name`. `is_empty()` includes `name`. +- `lua.rs`, `txt.rs`, `csv.rs`: All use filename stem + meta override. No lingering `encode_path_name`/`decode_path_name`. +- `json_model.rs`: Top-level `name` (with `alias = "Name"` for legacy) correctly overrides filename. `specified_name` set on metadata. +- `mod.rs`: `decode_path_name` fully removed. + +### Dedup key consistency (pass 2) + +All 6 insertion sites use `.to_lowercase()` consistently: + +1. `file_names.rs` `deduplicate_name()` -- internal comparison +2. `dir.rs` -- 3 insertion points + pre-seed +3. `project.rs` -- insertion + pre-seed +4. `api.rs` -- seed from tree siblings + insert after add + `process_children_incremental` + +Cross-format collision (file vs directory with same slug) handled correctly. + +### Property/data preservation (pass 2) + +- `Name` property vs instance name: Separate fields, no cross-contamination. +- Ref properties: Resolved via DOM tree paths, not filenames. +- Attributes: `upsert_meta_name` merges (only touches `"name"` key), no clobber risk. +- Script source through renames: `fs::rename` preserves content. + +### Legacy cleanup (pass 2) + +- `path_encoding.rs`: Deleted, zero references. +- `%ENCODED%` patterns: Zero in source/tests/snaps (only in `.cursor/plans/` docs). +- `deny_unknown_fields`: Removed from `SyncbackRules`. +- Old project files: Parse without crashing (serde ignores unknown fields on `SyncbackRules`). + +### Meta file lifecycle (pass 2) + +- Creation: Gated by `needs_meta`. Merge-based (only touches `"name"` key). +- Update: C1 fix ensures meta updates even when slug doesn't change. +- Deletion: `remove_meta_name` deletes empty file, rewrites when other fields remain. +- Orphaned meta: Not loaded as instance (excluded from sync rules), benign on disk. +- Model JSON vs adjacent meta: No conflict -- `JsonModel` doesn't read adjacent meta. + diff --git a/plugin/src/App/Components/PatchVisualizer/DomLabel.lua b/plugin/src/App/Components/PatchVisualizer/DomLabel.lua index 8dd78716..f804cad7 100644 --- a/plugin/src/App/Components/PatchVisualizer/DomLabel.lua +++ b/plugin/src/App/Components/PatchVisualizer/DomLabel.lua @@ -112,11 +112,10 @@ local function DiffBar(props) }), } - if removed > 0 then - local color = if props.isWhitespaceOnly then theme.Diff.WhitespaceOnly else theme.Diff.Remove - children.SegRemove = e("Frame", { - Size = UDim2.fromScale(removed / total, 1), - BackgroundColor3 = color, + if propChanges > 0 then + children.SegProp = e("Frame", { + Size = UDim2.fromScale(propChanges / total, 1), + BackgroundColor3 = theme.Diff.Property, BackgroundTransparency = props.transparency, BorderSizePixel = 0, LayoutOrder = 1, @@ -134,10 +133,11 @@ local function DiffBar(props) }) end - if propChanges > 0 then - children.SegProp = e("Frame", { - Size = UDim2.fromScale(propChanges / total, 1), - BackgroundColor3 = theme.Diff.Property, + if removed > 0 then + local color = if props.isWhitespaceOnly then theme.Diff.WhitespaceOnly else theme.Diff.Remove + children.SegRemove = e("Frame", { + Size = UDim2.fromScale(removed / total, 1), + BackgroundColor3 = color, BackgroundTransparency = props.transparency, BorderSizePixel = 0, LayoutOrder = 3, @@ -678,7 +678,7 @@ function DomLabel:render() then theme.Diff.WhitespaceOnly else theme.Diff.Remove, transparency = props.transparency, - layoutOrder = 1, + layoutOrder = 3, tooltipText = (props.changeInfo.linesRemoved or 0) .. (if (props.changeInfo.linesRemoved or 0) == 1 then " line removed" else " lines removed") .. (if props.changeInfo.isWhitespaceOnly then " (whitespace only)" else ""), @@ -707,7 +707,7 @@ function DomLabel:render() text = props.changeInfo.propChanges .. "P", color = theme.Diff.Property, transparency = props.transparency, - layoutOrder = 3, + layoutOrder = 1, tooltipText = props.changeInfo.propChanges .. (if props.changeInfo.propChanges == 1 then " property change" else " property changes") .. " (excluding Source)", diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__dedup_suffix_with_meta.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__dedup_suffix_with_meta.snap new file mode 100644 index 00000000..959d0509 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__dedup_suffix_with_meta.snap @@ -0,0 +1,25 @@ +--- +source: tests/tests/build.rs +expression: contents +--- + + + + dedup_suffix_with_meta + + + + A/B + + + + + + A_B + + + + + diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_name_override.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_name_override.snap new file mode 100644 index 00000000..5bafeb1d --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_name_override.snap @@ -0,0 +1,18 @@ +--- +source: tests/tests/build.rs +expression: contents +--- + + + + init_meta_name_override + + + + Real:Name + + + + + diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__meta_name_override.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__meta_name_override.snap new file mode 100644 index 00000000..6ce5ef33 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__meta_name_override.snap @@ -0,0 +1,18 @@ +--- +source: tests/tests/build.rs +expression: contents +--- + + + + meta_name_override + + + + Hey/Bro + + + + + diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__model_json_name_override.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__model_json_name_override.snap new file mode 100644 index 00000000..49c98178 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__model_json_name_override.snap @@ -0,0 +1,16 @@ +--- +source: tests/tests/build.rs +expression: contents +--- + + + + model_json_name_override + + + + Hey/Bro + + + + diff --git a/rojo-test/build-tests/dedup_suffix_with_meta/default.project.json5 b/rojo-test/build-tests/dedup_suffix_with_meta/default.project.json5 new file mode 100644 index 00000000..8f78e37f --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_with_meta/default.project.json5 @@ -0,0 +1,6 @@ +{ + "name": "dedup_suffix_with_meta", + "tree": { + "$path": "src" + } +} diff --git a/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo.luau b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo.luau new file mode 100644 index 00000000..6bb0d45c --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo.luau @@ -0,0 +1 @@ +return "first" diff --git a/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo.meta.json5 b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo.meta.json5 new file mode 100644 index 00000000..850bdcd2 --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo.meta.json5 @@ -0,0 +1,3 @@ +{ + "name": "A/B" +} diff --git a/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo~1.luau b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo~1.luau new file mode 100644 index 00000000..70a0843e --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo~1.luau @@ -0,0 +1 @@ +return "second" diff --git a/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo~1.meta.json5 b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo~1.meta.json5 new file mode 100644 index 00000000..b242144e --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_with_meta/src/Foo~1.meta.json5 @@ -0,0 +1,3 @@ +{ + "name": "A_B" +} diff --git a/rojo-test/build-tests/init_meta_name_override/default.project.json5 b/rojo-test/build-tests/init_meta_name_override/default.project.json5 new file mode 100644 index 00000000..73026654 --- /dev/null +++ b/rojo-test/build-tests/init_meta_name_override/default.project.json5 @@ -0,0 +1,6 @@ +{ + "name": "init_meta_name_override", + "tree": { + "$path": "src" + } +} diff --git a/rojo-test/build-tests/init_meta_name_override/src/MyFolder/init.luau b/rojo-test/build-tests/init_meta_name_override/src/MyFolder/init.luau new file mode 100644 index 00000000..9d132fdf --- /dev/null +++ b/rojo-test/build-tests/init_meta_name_override/src/MyFolder/init.luau @@ -0,0 +1 @@ +return "init module" diff --git a/rojo-test/build-tests/init_meta_name_override/src/MyFolder/init.meta.json5 b/rojo-test/build-tests/init_meta_name_override/src/MyFolder/init.meta.json5 new file mode 100644 index 00000000..2f8525dd --- /dev/null +++ b/rojo-test/build-tests/init_meta_name_override/src/MyFolder/init.meta.json5 @@ -0,0 +1,3 @@ +{ + "name": "Real:Name" +} diff --git a/rojo-test/build-tests/meta_name_override/default.project.json5 b/rojo-test/build-tests/meta_name_override/default.project.json5 new file mode 100644 index 00000000..ba216bda --- /dev/null +++ b/rojo-test/build-tests/meta_name_override/default.project.json5 @@ -0,0 +1,6 @@ +{ + "name": "meta_name_override", + "tree": { + "$path": "src" + } +} diff --git a/rojo-test/build-tests/meta_name_override/src/Hey_Bro.luau b/rojo-test/build-tests/meta_name_override/src/Hey_Bro.luau new file mode 100644 index 00000000..13fddb8b --- /dev/null +++ b/rojo-test/build-tests/meta_name_override/src/Hey_Bro.luau @@ -0,0 +1 @@ +return "module" diff --git a/rojo-test/build-tests/meta_name_override/src/Hey_Bro.meta.json5 b/rojo-test/build-tests/meta_name_override/src/Hey_Bro.meta.json5 new file mode 100644 index 00000000..578043b8 --- /dev/null +++ b/rojo-test/build-tests/meta_name_override/src/Hey_Bro.meta.json5 @@ -0,0 +1,3 @@ +{ + "name": "Hey/Bro" +} diff --git a/rojo-test/build-tests/model_json_name_override/default.project.json5 b/rojo-test/build-tests/model_json_name_override/default.project.json5 new file mode 100644 index 00000000..925cc9c6 --- /dev/null +++ b/rojo-test/build-tests/model_json_name_override/default.project.json5 @@ -0,0 +1,6 @@ +{ + "name": "model_json_name_override", + "tree": { + "$path": "src" + } +} diff --git a/rojo-test/build-tests/model_json_name_override/src/Hey_Bro.model.json5 b/rojo-test/build-tests/model_json_name_override/src/Hey_Bro.model.json5 new file mode 100644 index 00000000..fb6ec150 --- /dev/null +++ b/rojo-test/build-tests/model_json_name_override/src/Hey_Bro.model.json5 @@ -0,0 +1,4 @@ +{ + "name": "Hey/Bro", + "className": "Configuration" +} diff --git a/src/change_processor.rs b/src/change_processor.rs index 4ad21e7b..251fc617 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -15,7 +15,7 @@ use crate::{ apply_patch_set, compute_patch_set, AppliedPatchSet, InstigatingSource, PatchSet, RojoTree, }, snapshot_middleware::{snapshot_from_vfs, snapshot_project_node}, - syncback::{name_needs_slugify, slugify_name}, + syncback::{deduplicate_name, name_needs_slugify, slugify_name, strip_script_suffix}, }; /// Processes file change events, updates the DOM, and sends those updates @@ -226,6 +226,13 @@ impl JobThreadContext { } } + /// Suppress the next Remove VFS event for the given path. + fn suppress_path_remove(&self, path: &Path) { + let mut suppressed = self.suppressed_paths.lock().unwrap(); + let key = Self::suppression_key(path); + suppressed.entry(key).or_insert((0, 0)).0 += 1; + } + /// Upsert the `name` field in a `.meta.json5` file, suppressing filesystem /// events to avoid feedback loops. fn upsert_meta_name_field(&self, meta_path: &Path, real_name: &str) { @@ -251,10 +258,11 @@ impl JobThreadContext { self.unsuppress_path(meta_path); } Ok(RemoveNameOutcome::FileDeleted) => { - // File was deleted — also suppress the Remove event. - // suppress_path already covers Create/Write; we need - // suppress_path_any for the Remove event as well. - self.suppress_path_any(meta_path); + // File was deleted, not rewritten. Swap: undo the + // pre-emptive Write suppression and add a Remove + // suppression instead so the counts are (1, 0). + self.unsuppress_path(meta_path); + self.suppress_path_remove(meta_path); } Ok(RemoveNameOutcome::FieldRemoved) => { // File was rewritten — suppress_path already covers it. @@ -606,7 +614,7 @@ impl JobThreadContext { let mut tree = self.tree.lock().unwrap(); // NOTE: We do NOT delete files from disk here. The API handler - // (handle_api_write → syncback_removed_instance) already deleted + // (handle_api_write → remove_instance_at_path) already deleted // the files before sending this PatchSet. Re-deleting would // destroy any new file created at the same path — e.g., when an // instance is renamed from "joe_test" to "joe/test", the new slug @@ -688,6 +696,11 @@ impl JobThreadContext { } else { new_name.to_string() }; + // Directory renames: fs::rename fails safely + // on all platforms if the target is a non-empty + // directory, so no dedup is needed here (unlike + // the file rename path below, where Unix + // fs::rename silently overwrites). let new_dir_path = grandparent.join(&slugified_new_name); @@ -791,12 +804,57 @@ impl JobThreadContext { } else { new_name.to_string() }; + + // Guard against rename collision: if the target + // path already exists and isn't our own file, + // deduplicate the name against siblings. + let deduped_new_name; + { + let candidate_file = if extension.is_empty() { + format!("{}{}", slugified_new_name, script_suffix) + } else { + format!( + "{}{}.{}", + slugified_new_name, script_suffix, extension + ) + }; + let candidate = parent.join(&candidate_file); + if candidate != *path && candidate.exists() { + let mut taken: std::collections::HashSet = + std::collections::HashSet::new(); + if let Ok(entries) = fs::read_dir(parent) { + for entry in entries.flatten() { + let ep = entry.path(); + let slug = if ep.is_dir() { + ep.file_name() + .and_then(|f| f.to_str()) + .unwrap_or("") + .to_lowercase() + } else { + let s = ep + .file_stem() + .and_then(|f| f.to_str()) + .unwrap_or(""); + strip_script_suffix(s).to_lowercase() + }; + taken.insert(slug); + } + } + // Free the slot we're vacating + taken.remove(&old_base.to_lowercase()); + deduped_new_name = + deduplicate_name(&slugified_new_name, &taken); + } else { + deduped_new_name = slugified_new_name.clone(); + } + } + let new_file_name = if extension.is_empty() { - format!("{}{}", slugified_new_name, script_suffix) + format!("{}{}", deduped_new_name, script_suffix) } else { format!( "{}{}.{}", - slugified_new_name, script_suffix, extension + deduped_new_name, script_suffix, extension ) }; let new_path = parent.join(&new_file_name); @@ -828,7 +886,7 @@ impl JobThreadContext { .join(format!("{}.meta.json5", old_base)); let new_meta = parent.join(format!( "{}.meta.json5", - slugified_new_name + deduped_new_name )); if old_meta.exists() { self.suppress_path_any(&old_meta); @@ -839,7 +897,7 @@ impl JobThreadContext { self.unsuppress_path(&new_meta); } } - effective_meta_base = &slugified_new_name; + effective_meta_base = &deduped_new_name; } } else { effective_meta_base = old_base; @@ -848,12 +906,13 @@ impl JobThreadContext { // Always update the meta name field when the // instance name changed, even if the path didn't // change (e.g. "Foo/Bar" → "Foo|Bar" both slugify - // to "Foo_Bar"). + // to "Foo_Bar"). Use deduped_new_name because + // dedup may have appended ~N. let current_meta = parent.join(format!( "{}.meta.json5", effective_meta_base )); - if slugified_new_name != *new_name { + if deduped_new_name != *new_name { self.upsert_meta_name_field( ¤t_meta, new_name, diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index e94ab12a..2a5e0c3b 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -77,10 +77,16 @@ pub fn snapshot_csv_init( let mut init_snapshot = snapshot_csv(context, vfs, init_path, &dir_snapshot.name)?.unwrap(); + // Preserve the init script's instigating_source (the actual file path) + // before copying the directory's metadata (which has the folder path) + let script_instigating_source = init_snapshot.metadata.instigating_source.take(); + init_snapshot.children = dir_snapshot.children; init_snapshot.metadata = dir_snapshot.metadata; - // The directory snapshot middleware includes all possible init paths - // so we don't need to add it here. + + // Restore the init script's instigating_source so two-way sync writes + // to the actual file (e.g., init.csv) instead of the directory + init_snapshot.metadata.instigating_source = script_instigating_source; DirectoryMetadata::read_and_apply_all(vfs, folder_path, &mut init_snapshot)?; diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index 8794640d..e26084cd 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -8,9 +8,10 @@ use memofs::{DirEntry, Vfs}; use crate::{ snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource}, + snapshot_middleware::Middleware, syncback::{ - hash_instance, name_needs_slugify, slugify_name, FsSnapshot, SyncbackReturn, - SyncbackSnapshot, + hash_instance, name_needs_slugify, slugify_name, strip_middleware_extension, FsSnapshot, + SyncbackReturn, SyncbackSnapshot, }, }; @@ -186,25 +187,49 @@ pub fn syncback_dir_no_meta<'sync>( old_child_map.insert(inst.name(), inst); } - // Pre-seed taken_names from old children's slugified instance names - // so that new-only children correctly dedup against existing siblings - // (e.g., new "A/B" → slug "A_B" won't collide with existing "A_B"). + // Pre-seed taken_names from old children's actual filesystem dedup keys + // so that new-only children correctly dedup against existing siblings. + // We derive keys from relevant_paths (the real filename on disk) rather + // than slugifying instance names, because an old instance may already + // have a tilde suffix (e.g. A_B~1.luau from prior dedup) that the bare + // slug wouldn't capture. // Only in incremental mode: in clean mode, old_ref is forced to None // for all children, so every child is treated as new and pre-seeding // would cause false collisions (spurious ~1 suffixes). if snapshot.data.is_incremental() { for inst in old_child_map.values() { - let name = inst.name(); - let slug = if name_needs_slugify(name) { - slugify_name(name).to_lowercase() + if let Some(path) = inst.metadata().relevant_paths.first() { + if let Some(filename) = path.file_name().and_then(|f| f.to_str()) { + let middleware = inst.metadata().middleware.unwrap_or(Middleware::Dir); + let dedup_key = + strip_middleware_extension(filename, middleware).to_lowercase(); + taken_names.insert(dedup_key); + } } else { - name.to_lowercase() - }; - taken_names.insert(slug); + // Fallback (shouldn't happen for old instances with disk presence) + let name = inst.name(); + let slug = if name_needs_slugify(name) { + slugify_name(name).to_lowercase() + } else { + name.to_lowercase() + }; + taken_names.insert(slug); + } } } - for new_child_ref in new_inst.children() { + // Sort children alphabetically for deterministic dedup ordering. + // Without this, DOM iteration order determines which sibling gets + // the base slug vs ~1, causing non-deterministic output across + // different serializations of the same place. + let mut sorted_children: Vec<_> = new_inst + .children() + .iter() + .map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone())) + .collect(); + sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b)); + + for (new_child_ref, _) in &sorted_children { let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); // Skip children with duplicate names - cannot reliably sync them @@ -282,7 +307,15 @@ pub fn syncback_dir_no_meta<'sync>( })); } else { // There is no old instance. Just add every child. - for new_child_ref in new_inst.children() { + // Sort alphabetically for deterministic dedup ordering. + let mut sorted_children: Vec<_> = new_inst + .children() + .iter() + .map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone())) + .collect(); + sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b)); + + for (new_child_ref, _) in &sorted_children { let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); // Skip children with duplicate names - cannot reliably sync them diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index eb11ba43..483c04c7 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -21,8 +21,8 @@ use crate::{ }, snapshot_middleware::Middleware, syncback::{ - filter_properties, inst_path, name_needs_slugify, slugify_name, FsSnapshot, SyncbackReturn, - SyncbackSnapshot, + filter_properties, inst_path, name_needs_slugify, slugify_name, + strip_middleware_extension, FsSnapshot, SyncbackReturn, SyncbackSnapshot, }, variant_eq::variant_eq, RojoRef, @@ -658,9 +658,12 @@ pub fn syncback_project<'sync>( std::collections::HashSet, > = std::collections::HashMap::new(); - // Pre-seed taken_names from old children's slugified instance names - // so that new-only children correctly dedup against existing siblings - // (e.g., new "utils" won't collide with existing "Utils.luau"). + // Pre-seed taken_names from old children's actual filesystem dedup keys + // so that new-only children correctly dedup against existing siblings. + // We derive keys from relevant_paths (the real filename on disk) rather + // than slugifying instance names, because an old instance may already + // have a tilde suffix (e.g. A_B~1.luau from prior dedup) that the bare + // slug wouldn't capture. // Only in incremental mode: in clean mode, old_ref is forced to None // for all children, so every child is treated as new and pre-seeding // would cause false collisions (spurious ~1 suffixes). @@ -668,13 +671,24 @@ pub fn syncback_project<'sync>( if let Some(parent_path) = ref_to_path_map.get(&new_inst.referent()) { let taken = taken_names_per_dir.entry(parent_path.clone()).or_default(); for old_child in old_child_map.values() { - let name = old_child.name(); - let slug = if name_needs_slugify(name) { - slugify_name(name).to_lowercase() + if let Some(path) = old_child.metadata().relevant_paths.first() { + if let Some(filename) = path.file_name().and_then(|f| f.to_str()) { + let middleware = + old_child.metadata().middleware.unwrap_or(Middleware::Dir); + let dedup_key = + strip_middleware_extension(filename, middleware).to_lowercase(); + taken.insert(dedup_key); + } } else { - name.to_lowercase() - }; - taken.insert(slug); + // Fallback (shouldn't happen for old instances with disk presence) + let name = old_child.name(); + let slug = if name_needs_slugify(name) { + slugify_name(name).to_lowercase() + } else { + name.to_lowercase() + }; + taken.insert(slug); + } } } } diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap index d6e60fee..a28c445a 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init.snap @@ -6,7 +6,7 @@ snapshot_id: "00000000000000000000000000000000" metadata: ignore_unknown_instances: false instigating_source: - Path: /root + Path: /root/init.csv relevant_paths: - /root - /root/init.luau diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap index 2ff5b269..ada8a161 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_init_with_meta.snap @@ -6,7 +6,7 @@ snapshot_id: "00000000000000000000000000000000" metadata: ignore_unknown_instances: false instigating_source: - Path: /root + Path: /root/init.csv relevant_paths: - /root - /root/init.luau diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index d3f7b11f..9eaf45d1 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -2400,4 +2400,77 @@ mod tests { let dk_from_filename = strip_middleware_extension(&name1, Middleware::ModuleScript); assert_eq!(dk_from_filename.to_lowercase(), dk1.to_lowercase()); } + + #[test] + fn syncback_forbidden_chars_produce_slugified_filenames() { + // Verify that instances with forbidden chars produce correct + // (filename, needs_meta, dedup_key) via name_for_inst + let mut dom = rbx_dom_weak::WeakDom::new(rbx_dom_weak::InstanceBuilder::new("Folder")); + let root_ref = dom.root_ref(); + let child = dom.insert(root_ref, rbx_dom_weak::InstanceBuilder::new("ModuleScript").with_name("Hey/Bro")); + let inst = dom.get_by_ref(child).unwrap(); + let taken = std::collections::HashSet::new(); + let (filename, needs_meta, dedup_key) = name_for_inst( + crate::snapshot_middleware::Middleware::ModuleScript, + inst, None, &taken, + ).unwrap(); + assert_eq!(filename.as_ref(), "Hey_Bro.luau"); + assert!(needs_meta); + assert_eq!(dedup_key, "Hey_Bro"); + } + + #[test] + fn syncback_collision_deduplicates() { + let mut dom = rbx_dom_weak::WeakDom::new(rbx_dom_weak::InstanceBuilder::new("Folder")); + let root_ref = dom.root_ref(); + let child1 = dom.insert(root_ref, rbx_dom_weak::InstanceBuilder::new("ModuleScript").with_name("A/B")); + let child2 = dom.insert(root_ref, rbx_dom_weak::InstanceBuilder::new("ModuleScript").with_name("A:B")); + let inst1 = dom.get_by_ref(child1).unwrap(); + let inst2 = dom.get_by_ref(child2).unwrap(); + + let mut taken = std::collections::HashSet::new(); + let (f1, m1, dk1) = name_for_inst( + crate::snapshot_middleware::Middleware::ModuleScript, + inst1, None, &taken, + ).unwrap(); + taken.insert(dk1.to_lowercase()); + let (f2, m2, dk2) = name_for_inst( + crate::snapshot_middleware::Middleware::ModuleScript, + inst2, None, &taken, + ).unwrap(); + + assert_eq!(f1.as_ref(), "A_B.luau"); + assert!(m1); + assert_eq!(f2.as_ref(), "A_B~1.luau"); + assert!(m2); + assert_ne!(dk1.to_lowercase(), dk2.to_lowercase()); + } + + #[test] + fn syncback_idempotency_same_input_same_output() { + // Run the same set of instances through name_for_inst twice + // and verify identical output (determinism). + let names = vec!["Alpha", "Beta/Gamma", "Beta:Gamma", "Delta", "CON"]; + let mw = crate::snapshot_middleware::Middleware::ModuleScript; + + let mut dom = rbx_dom_weak::WeakDom::new(rbx_dom_weak::InstanceBuilder::new("Folder")); + let root = dom.root_ref(); + let refs: Vec<_> = names.iter().map(|n| + dom.insert(root, rbx_dom_weak::InstanceBuilder::new("ModuleScript").with_name(*n)) + ).collect(); + + let run = |dom: &rbx_dom_weak::WeakDom, refs: &[rbx_dom_weak::types::Ref]| -> Vec<(String, bool, String)> { + let mut taken = std::collections::HashSet::new(); + refs.iter().map(|r| { + let inst = dom.get_by_ref(*r).unwrap(); + let (f, m, dk) = name_for_inst(mw, inst, None, &taken).unwrap(); + taken.insert(dk.to_lowercase()); + (f.into_owned(), m, dk) + }).collect() + }; + + let result1 = run(&dom, &refs); + let result2 = run(&dom, &refs); + assert_eq!(result1, result2, "Two runs of name_for_inst must produce identical output"); + } } diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 4586489f..0ca5c123 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -32,7 +32,8 @@ use crate::{ pub use file_names::{ adjacent_meta_path, deduplicate_name, extension_for_middleware, name_for_inst, - name_needs_slugify, slugify_name, strip_middleware_extension, validate_file_name, + name_needs_slugify, slugify_name, strip_middleware_extension, strip_script_suffix, + validate_file_name, }; pub use fs_snapshot::FsSnapshot; pub use hash::*; diff --git a/src/web/api.rs b/src/web/api.rs index 6028decf..d5ddb02d 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -354,7 +354,7 @@ impl ApiService { // they are NOT removed from the in-memory tree. let mut actually_removed: Vec = Vec::new(); if !request.removed.is_empty() { - let removal_actions: Vec<(Ref, Option<(PathBuf, bool)>)> = { + let removal_actions: Vec<(Ref, Option)> = { let tree = self.serve_session.tree(); request .removed @@ -364,7 +364,7 @@ impl ApiService { inst.metadata().instigating_source.as_ref().and_then(|src| { match src { crate::snapshot::InstigatingSource::Path(p) => { - Some((p.clone(), p.is_dir())) + Some(p.clone()) } crate::snapshot::InstigatingSource::ProjectNode { name, @@ -386,66 +386,9 @@ impl ApiService { // Phase 2: Execute filesystem deletions without the lock for (id, action) in removal_actions { - if let Some((path, is_dir)) = action { - if !path.exists() { - log::info!( - "Syncback: Path already removed (likely parent was deleted): {}", - path.display() - ); + if let Some(path) = action { + if self.remove_instance_at_path(&path) { actually_removed.push(id); - continue; - } - if is_dir { - self.suppress_path_remove(&path); - if let Err(err) = fs::remove_dir_all(&path) { - log::warn!( - "Failed to remove directory {:?} for instance {:?}: {}", - path, - id, - err - ); - } else { - log::info!("Syncback: Removed directory at {}", path.display()); - actually_removed.push(id); - } - } else { - self.suppress_path_remove(&path); - if let Err(err) = fs::remove_file(&path) { - log::warn!( - "Failed to remove file {:?} for instance {:?}: {}", - path, - id, - err - ); - } else { - log::info!("Syncback: Removed file at {}", path.display()); - actually_removed.push(id); - } - // Also remove adjacent meta file. - // Strip known script suffixes (.server, .client, etc.) - // rather than splitting on dots, so that names containing - // dots (e.g. "Config.Client.server.luau") resolve correctly. - if let Some(file_stem) = path.file_stem().and_then(|s| s.to_str()) { - let base_name = file_stem - .strip_suffix(".server") - .or_else(|| file_stem.strip_suffix(".client")) - .or_else(|| file_stem.strip_suffix(".plugin")) - .or_else(|| file_stem.strip_suffix(".local")) - .or_else(|| file_stem.strip_suffix(".legacy")) - .unwrap_or(file_stem); - if let Some(parent_dir) = path.parent() { - let meta_path = - parent_dir.join(format!("{}.meta.json5", base_name)); - if meta_path.exists() { - self.suppress_path_remove(&meta_path); - let _ = fs::remove_file(&meta_path); - log::info!( - "Syncback: Removed adjacent meta file at {}", - meta_path.display() - ); - } - } - } } } } @@ -1422,139 +1365,109 @@ impl ApiService { Ok(new_dir) } - /// Syncback a removed instance by deleting its file(s) from the filesystem. - /// This is called when the user selects "pull" for an instance that exists in Rojo - /// but has been deleted in Studio. - #[allow(dead_code)] - fn syncback_removed_instance( - &self, - removed_id: Ref, - tree: &crate::snapshot::RojoTree, - ) -> anyhow::Result<()> { - use anyhow::Context; + /// Remove an instance from the filesystem given its path. + /// + /// Handles three cases: + /// - Directory: `remove_dir_all` + /// - Init file (`init.luau`, `init.server.luau`, etc.): remove parent directory + /// (which IS the instance) + grandparent-level adjacent meta + /// - Regular file: remove the file + adjacent `.meta.json5` + /// + /// Returns `true` if the removal succeeded (or the path was already gone), + /// `false` if the filesystem operation failed (logged as warning). + fn remove_instance_at_path(&self, path: &Path) -> bool { + use crate::syncback::adjacent_meta_path; - // Find the instance in the tree - let instance = tree - .get_instance(removed_id) - .context("Instance not found in Rojo tree")?; - - // Get the instigating source and handle each variant appropriately - let instigating_source = instance - .metadata() - .instigating_source - .as_ref() - .context("Instance has no filesystem path (not synced from filesystem)")?; - - // Only delete instances that originate from filesystem paths, not from project nodes - let instance_path = match instigating_source { - InstigatingSource::Path(path) => path, - InstigatingSource::ProjectNode { name, .. } => { - // Instances defined in project files cannot be removed via syncback - // because that would require modifying the project file structure - log::warn!( - "Syncback: Cannot remove instance '{}' (id: {:?}) - it is defined in a project file", - name, - removed_id - ); - anyhow::bail!( - "Cannot remove instance '{}' because it is defined in a project file. \ - To remove this instance, edit the project file directly.", - name - ); - } - }; - - // Check if path exists - it may have already been deleted as part of a parent removal - // (e.g., if both a directory and its children are marked for removal, the directory - // deletion will recursively delete all children, so we just skip them) - if !instance_path.exists() { - log::trace!( + if !path.exists() { + log::info!( "Syncback: Path already removed (likely parent was deleted): {}", - instance_path.display() + path.display() ); - return Ok(()); + return true; } - // Delete the file or directory - if instance_path.is_dir() { - self.suppress_path_remove(instance_path); - fs::remove_dir_all(instance_path).with_context(|| { - format!("Failed to remove directory: {}", instance_path.display()) - })?; - log::info!("Syncback: Removed directory at {}", instance_path.display()); - } else if instance_path.is_file() { - use crate::syncback::adjacent_meta_path; + if path.is_dir() { + self.suppress_path_remove(path); + if let Err(err) = fs::remove_dir_all(path) { + log::warn!("Failed to remove directory {:?}: {}", path, err); + return false; + } + log::info!("Syncback: Removed directory at {}", path.display()); + return true; + } - let file_name = instance_path - .file_name() - .and_then(|f| f.to_str()) - .unwrap_or(""); - let is_init_file = file_name.starts_with("init."); + // File: check if it's an init file (directory-format script) + let file_name = path.file_name().and_then(|f| f.to_str()).unwrap_or(""); + let is_init_file = file_name.starts_with("init."); - if is_init_file { - // Directory-format script: the parent directory represents the - // instance (e.g. src/MyModule/init.luau → the directory IS MyModule). - // Remove the entire directory. - let dir_path = instance_path.parent().unwrap(); - self.suppress_path_remove(dir_path); - fs::remove_dir_all(dir_path).with_context(|| { - format!( - "Failed to remove directory for init file: {}", - dir_path.display() - ) - })?; - log::info!( - "Syncback: Removed directory {} (init file: {})", - dir_path.display(), - instance_path.display() + if is_init_file { + // Directory-format script: the parent directory represents the + // instance (e.g. src/MyModule/init.luau → the directory IS MyModule). + // Remove the entire directory. + let dir_path = path.parent().unwrap(); + self.suppress_path_remove(dir_path); + if let Err(err) = fs::remove_dir_all(dir_path) { + log::warn!( + "Failed to remove directory for init file {:?}: {}", + dir_path, + err ); + return false; + } + log::info!( + "Syncback: Removed directory {} (init file: {})", + dir_path.display(), + path.display() + ); - // Also remove adjacent dir-level meta in the grandparent if it - // exists (e.g. grandparent/MyModule.meta.json5). - if let Some(grandparent) = dir_path.parent() { - if let Some(dir_name) = dir_path.file_name().and_then(|f| f.to_str()) { - let dir_meta = grandparent.join(format!("{}.meta.json5", dir_name)); - if dir_meta.exists() { - if let Err(err) = fs::remove_file(&dir_meta) { - log::warn!( - "Failed to remove dir-level meta file {}: {}", - dir_meta.display(), - err - ); - } + // Also remove adjacent dir-level meta in the grandparent if it + // exists (e.g. grandparent/MyModule.meta.json5). + if let Some(grandparent) = dir_path.parent() { + if let Some(dir_name) = dir_path.file_name().and_then(|f| f.to_str()) { + let dir_meta = grandparent.join(format!("{}.meta.json5", dir_name)); + if dir_meta.exists() { + self.suppress_path_remove(&dir_meta); + if let Err(err) = fs::remove_file(&dir_meta) { + log::warn!( + "Failed to remove dir-level meta file {}: {}", + dir_meta.display(), + err + ); } } } - } else { - // Regular file: remove the file itself - self.suppress_path_remove(instance_path); - fs::remove_file(instance_path).with_context(|| { - format!("Failed to remove file: {}", instance_path.display()) - })?; - log::info!("Syncback: Removed file at {}", instance_path.display()); + } + } else { + // Regular file: remove the file itself + self.suppress_path_remove(path); + if let Err(err) = fs::remove_file(path) { + log::warn!("Failed to remove file {:?}: {}", path, err); + return false; + } + log::info!("Syncback: Removed file at {}", path.display()); - // Remove the adjacent meta file if it exists. The meta file is - // named after the script file's base stem (the slugified name), - // not the raw instance name. - let meta_path = adjacent_meta_path(instance_path); - if meta_path.exists() { - if let Err(err) = fs::remove_file(&meta_path) { - log::warn!( - "Failed to remove adjacent meta file {}: {}", - meta_path.display(), - err - ); - } else { - log::info!( - "Syncback: Removed adjacent meta file at {}", - meta_path.display() - ); - } + // Remove the adjacent meta file if it exists. The meta file is + // named after the script file's base stem (the slugified name), + // not the raw instance name. + let meta_path = adjacent_meta_path(path); + if meta_path.exists() { + self.suppress_path_remove(&meta_path); + if let Err(err) = fs::remove_file(&meta_path) { + log::warn!( + "Failed to remove adjacent meta file {}: {}", + meta_path.display(), + err + ); + } else { + log::info!( + "Syncback: Removed adjacent meta file at {}", + meta_path.display() + ); } } } - Ok(()) + true } /// Check for duplicate names among children and return the set of unique children diff --git a/tests/tests/build.rs b/tests/tests/build.rs index 403cf7e7..6a936170 100644 --- a/tests/tests/build.rs +++ b/tests/tests/build.rs @@ -65,6 +65,10 @@ gen_build_tests! { no_name_project, no_name_top_level_project, tilde_no_meta, + meta_name_override, + model_json_name_override, + dedup_suffix_with_meta, + init_meta_name_override, } fn run_build_test(test_name: &str) { diff --git a/tests/tests/two_way_sync.rs b/tests/tests/two_way_sync.rs index 8e8d268f..832ed6f0 100644 --- a/tests/tests/two_way_sync.rs +++ b/tests/tests/two_way_sync.rs @@ -5156,3 +5156,87 @@ fn add_two_colliding_instances_deduplicates() { ); }); } + +/// Renaming a clean-named instance to a name with forbidden chars should +/// produce a slugified filename + adjacent meta with the `name` field. +#[test] +fn rename_clean_to_slugified() { + run_serve_test("syncback_encoded_names", |session, _redactions| { + let (session_id, normal_id) = get_encoded_names_instance(&session, "Normal"); + + let src = session.path().join("src"); + let old_path = src.join("Normal.luau"); + assert_file_exists(&old_path, "Normal.luau before rename"); + + send_update( + &session, + &session_id, + InstanceUpdate { + id: normal_id, + changed_name: Some("Hey/Bro".to_string()), + changed_class_name: None, + changed_properties: UstrMap::default(), + changed_metadata: None, + }, + ); + + let new_path = src.join("Hey_Bro.luau"); + let meta_path = src.join("Hey_Bro.meta.json5"); + + poll_file_exists(&new_path, "Hey_Bro.luau after rename"); + poll_not_exists(&old_path, "Normal.luau should be gone after rename"); + + assert_file_exists(&meta_path, "Meta file for Hey/Bro with name field"); + let meta_content = fs::read_to_string(&meta_path).unwrap(); + assert!( + meta_content.contains("\"Hey/Bro\""), + "Meta should contain the real instance name \"Hey/Bro\", got: {}", + meta_content + ); + }); +} + +/// Renaming a slugified instance back to a clean name should remove the +/// meta `name` field (or the entire meta file if it had no other fields). +#[test] +fn rename_slugified_to_clean() { + run_serve_test("syncback_encoded_names", |session, _redactions| { + let (session_id, encoded_id) = get_encoded_names_instance(&session, "What?Module"); + + let src = session.path().join("src"); + let old_path = src.join("What_Module.luau"); + let old_meta = src.join("What_Module.meta.json5"); + assert_file_exists(&old_path, "What_Module.luau before rename"); + assert_file_exists(&old_meta, "What_Module.meta.json5 before rename"); + + send_update( + &session, + &session_id, + InstanceUpdate { + id: encoded_id, + changed_name: Some("CleanName".to_string()), + changed_class_name: None, + changed_properties: UstrMap::default(), + changed_metadata: None, + }, + ); + + let new_path = src.join("CleanName.luau"); + let new_meta = src.join("CleanName.meta.json5"); + + poll_file_exists(&new_path, "CleanName.luau after rename"); + poll_not_exists(&old_path, "What_Module.luau should be gone after rename"); + poll_not_exists(&old_meta, "What_Module.meta.json5 should be gone after rename"); + + // The new meta file should either not exist (clean name needs no + // name override) or, if it exists, should NOT contain a "name" field. + if new_meta.exists() { + let meta_content = fs::read_to_string(&new_meta).unwrap(); + assert!( + !meta_content.contains("\"name\""), + "CleanName.meta.json5 should not have a \"name\" field, got: {}", + meta_content + ); + } + }); +}