hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 05:51:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Refactor API file handling and improve echo suppression documentation

Browse source 
- Updated the `write_init_meta_json` method to include a new parameter for better control over initialization behavior.
- Removed unnecessary calls to `suppress_path` and `suppress_path_remove` in various sections to streamline file handling.
- Revised comments in the `two_way_sync` tests to clarify the behavior of echo suppression during API interactions, ensuring accurate expectations for VFS event handling.

These changes enhance the clarity and efficiency of the API service and improve the documentation for testing echo suppression.
This commit is contained in:
Joe 2026-02-11 19:08:26 -06:00
parent e2f515d8eb
commit ec7f91017b
2 changed files with 22 additions and 50 deletions
Download patch file Download diff file Expand all files Collapse all files

43
src/web/api.rs
View file

@ -1091,7 +1091,7 @@ impl ApiService {
} else { } else {
None None
}; };
self.write_init_meta_json(existing_path, added, instance_name)?; self.write_init_meta_json(existing_path, added, instance_name, true)?;
log::info!( log::info!(
"Syncback: Updated existing {} at {}/init.meta.json5", "Syncback: Updated existing {} at {}/init.meta.json5",
class_name, class_name,
@ -1736,23 +1736,19 @@ impl ApiService {
added.name added.name
); );
if old_path.exists() { if old_path.exists() {
self.suppress_path_remove(old_path);
let _ = fs::remove_file(old_path); let _ = fs::remove_file(old_path);
} }
let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name)); let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name));
if meta_path.exists() { if meta_path.exists() {
self.suppress_path_remove(&meta_path);
let _ = fs::remove_file(&meta_path); let _ = fs::remove_file(&meta_path);
} }
} }
let dir_path = parent_dir.join(&encoded_name); let dir_path = parent_dir.join(&encoded_name);
self.suppress_path(&dir_path);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
let init_path = dir_path.join("init.luau"); let init_path = dir_path.join("init.luau");
self.suppress_path(&init_path);
fs::write(&init_path, source.as_bytes()).with_context(|| { fs::write(&init_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
@ -1761,7 +1757,6 @@ impl ApiService {
self.process_children_incremental(&unique_children, &dir_path, stats)?; self.process_children_incremental(&unique_children, &dir_path, stats)?;
} else { } else {
let file_path = parent_dir.join(format!("{}.luau", encoded_name)); let file_path = parent_dir.join(format!("{}.luau", encoded_name));
self.suppress_path(&file_path);
fs::write(&file_path, source.as_bytes()).with_context(|| { fs::write(&file_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
@ -1791,23 +1786,19 @@ impl ApiService {
added.name added.name
); );
if old_path.exists() { if old_path.exists() {
self.suppress_path_remove(old_path);
let _ = fs::remove_file(old_path); let _ = fs::remove_file(old_path);
} }
let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name)); let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name));
if meta_path.exists() { if meta_path.exists() {
self.suppress_path_remove(&meta_path);
let _ = fs::remove_file(&meta_path); let _ = fs::remove_file(&meta_path);
} }
} }
let dir_path = parent_dir.join(&encoded_name); let dir_path = parent_dir.join(&encoded_name);
self.suppress_path(&dir_path);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
let init_path = dir_path.join(format!("init.{}.luau", script_suffix)); let init_path = dir_path.join(format!("init.{}.luau", script_suffix));
self.suppress_path(&init_path);
fs::write(&init_path, source.as_bytes()).with_context(|| { fs::write(&init_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
@ -1817,7 +1808,6 @@ impl ApiService {
} else { } else {
let file_path = let file_path =
parent_dir.join(format!("{}.{}.luau", encoded_name, script_suffix)); parent_dir.join(format!("{}.{}.luau", encoded_name, script_suffix));
self.suppress_path(&file_path);
fs::write(&file_path, source.as_bytes()).with_context(|| { fs::write(&file_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
@ -1846,23 +1836,19 @@ impl ApiService {
added.name added.name
); );
if old_path.exists() { if old_path.exists() {
self.suppress_path_remove(old_path);
let _ = fs::remove_file(old_path); let _ = fs::remove_file(old_path);
} }
let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name)); let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name));
if meta_path.exists() { if meta_path.exists() {
self.suppress_path_remove(&meta_path);
let _ = fs::remove_file(&meta_path); let _ = fs::remove_file(&meta_path);
} }
} }
let dir_path = parent_dir.join(&encoded_name); let dir_path = parent_dir.join(&encoded_name);
self.suppress_path(&dir_path);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
let init_path = dir_path.join("init.local.luau"); let init_path = dir_path.join("init.local.luau");
self.suppress_path(&init_path);
fs::write(&init_path, source.as_bytes()).with_context(|| { fs::write(&init_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
@ -1871,7 +1857,6 @@ impl ApiService {
self.process_children_incremental(&unique_children, &dir_path, stats)?; self.process_children_incremental(&unique_children, &dir_path, stats)?;
} else { } else {
let file_path = parent_dir.join(format!("{}.local.luau", encoded_name)); let file_path = parent_dir.join(format!("{}.local.luau", encoded_name));
self.suppress_path(&file_path);
fs::write(&file_path, source.as_bytes()).with_context(|| { fs::write(&file_path, source.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
@ -1889,7 +1874,6 @@ impl ApiService {
"Folder" | "Configuration" | "Tool" | "ScreenGui" | "SurfaceGui" | "BillboardGui" "Folder" | "Configuration" | "Tool" | "ScreenGui" | "SurfaceGui" | "BillboardGui"
| "AdGui" => { | "AdGui" => {
let dir_path = parent_dir.join(&encoded_name); let dir_path = parent_dir.join(&encoded_name);
self.suppress_path(&dir_path);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
@ -1906,7 +1890,6 @@ impl ApiService {
// Uses !has_children which accounts for filtered duplicate children // Uses !has_children which accounts for filtered duplicate children
if !has_children && !has_metadata { if !has_children && !has_metadata {
let gitkeep = dir_path.join(".gitkeep"); let gitkeep = dir_path.join(".gitkeep");
self.suppress_path(&gitkeep);
fs::write(gitkeep, b"").with_context(|| "Failed to write .gitkeep")?; fs::write(gitkeep, b"").with_context(|| "Failed to write .gitkeep")?;
} }
@ -1925,11 +1908,10 @@ impl ApiService {
if has_children { if has_children {
// Must become directory - store StringValue data in init.meta.json5 // Must become directory - store StringValue data in init.meta.json5
let dir_path = parent_dir.join(&encoded_name); let dir_path = parent_dir.join(&encoded_name);
self.suppress_path(&dir_path);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
self.write_init_meta_json(&dir_path, added, meta_name_field)?; self.write_init_meta_json(&dir_path, added, meta_name_field, false)?;
log::info!( log::info!(
"Syncback: Created StringValue directory at {}", "Syncback: Created StringValue directory at {}",
dir_path.display() dir_path.display()
@ -1945,7 +1927,6 @@ impl ApiService {
}) })
.unwrap_or_default(); .unwrap_or_default();
let file_path = parent_dir.join(format!("{}.txt", encoded_name)); let file_path = parent_dir.join(format!("{}.txt", encoded_name));
self.suppress_path(&file_path);
fs::write(&file_path, value.as_bytes()).with_context(|| { fs::write(&file_path, value.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
@ -1960,7 +1941,6 @@ impl ApiService {
let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name)); let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name));
let content = crate::json::to_vec_pretty_sorted(&meta) let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize meta")?; .context("Failed to serialize meta")?;
self.suppress_path(&meta_path);
fs::write(&meta_path, &content).with_context(|| { fs::write(&meta_path, &content).with_context(|| {
format!("Failed to write meta: {}", meta_path.display()) format!("Failed to write meta: {}", meta_path.display())
})?; })?;
@ -1977,17 +1957,15 @@ impl ApiService {
if has_children { if has_children {
// Must become directory with init.csv // Must become directory with init.csv
let dir_path = parent_dir.join(&encoded_name); let dir_path = parent_dir.join(&encoded_name);
self.suppress_path(&dir_path);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
let init_path = dir_path.join("init.csv"); let init_path = dir_path.join("init.csv");
self.suppress_path(&init_path);
fs::write(&init_path, content.as_bytes()).with_context(|| { fs::write(&init_path, content.as_bytes()).with_context(|| {
format!("Failed to write file: {}", init_path.display()) format!("Failed to write file: {}", init_path.display())
})?; })?;
// Write init.meta.json5 for className and name preservation // Write init.meta.json5 for className and name preservation
self.write_init_meta_json(&dir_path, added, meta_name_field)?; self.write_init_meta_json(&dir_path, added, meta_name_field, false)?;
log::info!( log::info!(
"Syncback: Created LocalizationTable directory at {}", "Syncback: Created LocalizationTable directory at {}",
dir_path.display() dir_path.display()
@ -1995,7 +1973,6 @@ impl ApiService {
self.process_children_incremental(&unique_children, &dir_path, stats)?; self.process_children_incremental(&unique_children, &dir_path, stats)?;
} else { } else {
let file_path = parent_dir.join(format!("{}.csv", encoded_name)); let file_path = parent_dir.join(format!("{}.csv", encoded_name));
self.suppress_path(&file_path);
fs::write(&file_path, content.as_bytes()).with_context(|| { fs::write(&file_path, content.as_bytes()).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
@ -2010,7 +1987,6 @@ impl ApiService {
let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name)); let meta_path = parent_dir.join(format!("{}.meta.json5", encoded_name));
let content = crate::json::to_vec_pretty_sorted(&meta) let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize meta")?; .context("Failed to serialize meta")?;
self.suppress_path(&meta_path);
fs::write(&meta_path, &content).with_context(|| { fs::write(&meta_path, &content).with_context(|| {
format!("Failed to write meta: {}", meta_path.display()) format!("Failed to write meta: {}", meta_path.display())
})?; })?;
@ -2039,19 +2015,17 @@ impl ApiService {
added.name added.name
); );
if old_path.exists() { if old_path.exists() {
self.suppress_path_remove(old_path);
let _ = fs::remove_file(old_path); let _ = fs::remove_file(old_path);
} }
} }
let dir_path = parent_dir.join(&encoded_name); let dir_path = parent_dir.join(&encoded_name);
self.suppress_path(&dir_path);
fs::create_dir_all(&dir_path).with_context(|| { fs::create_dir_all(&dir_path).with_context(|| {
format!("Failed to create directory: {}", dir_path.display()) format!("Failed to create directory: {}", dir_path.display())
})?; })?;
// Write init.meta.json5 with class and properties // Write init.meta.json5 with class and properties
self.write_init_meta_json(&dir_path, added, meta_name_field)?; self.write_init_meta_json(&dir_path, added, meta_name_field, false)?;
log::info!( log::info!(
"Syncback: Updated {} at {}/init.meta.json5", "Syncback: Updated {} at {}/init.meta.json5",
@ -2069,7 +2043,6 @@ impl ApiService {
ExistingFileFormat::Standalone(p) => p.clone(), ExistingFileFormat::Standalone(p) => p.clone(),
_ => parent_dir.join(format!("{}.model.json5", encoded_name)), _ => parent_dir.join(format!("{}.model.json5", encoded_name)),
}; };
self.suppress_path(&file_path);
fs::write(&file_path, &content).with_context(|| { fs::write(&file_path, &content).with_context(|| {
format!("Failed to write file: {}", file_path.display()) format!("Failed to write file: {}", file_path.display())
})?; })?;
@ -2188,7 +2161,6 @@ impl ApiService {
let meta_path = dir_path.join("init.meta.json5"); let meta_path = dir_path.join("init.meta.json5");
let content = crate::json::to_vec_pretty_sorted(&meta) let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize init.meta.json5")?; .context("Failed to serialize init.meta.json5")?;
self.suppress_path(&meta_path);
fs::write(&meta_path, &content) fs::write(&meta_path, &content)
.with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?;
log::info!( log::info!(
@ -2221,7 +2193,6 @@ impl ApiService {
let meta_path = dir_path.join("init.meta.json5"); let meta_path = dir_path.join("init.meta.json5");
let content = crate::json::to_vec_pretty_sorted(&meta) let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize init.meta.json5")?; .context("Failed to serialize init.meta.json5")?;
self.suppress_path(&meta_path);
fs::write(&meta_path, &content) fs::write(&meta_path, &content)
.with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?;
log::info!( log::info!(
@ -2239,6 +2210,7 @@ impl ApiService {
dir_path: &std::path::Path, dir_path: &std::path::Path,
added: &crate::web::interface::AddedInstance, added: &crate::web::interface::AddedInstance,
instance_name: Option<&str>, instance_name: Option<&str>,
suppress_vfs: bool,
) -> anyhow::Result<()> { ) -> anyhow::Result<()> {
use anyhow::Context; use anyhow::Context;
@ -2255,7 +2227,9 @@ impl ApiService {
let meta_path = dir_path.join("init.meta.json5"); let meta_path = dir_path.join("init.meta.json5");
let content = crate::json::to_vec_pretty_sorted(&meta) let content = crate::json::to_vec_pretty_sorted(&meta)
.context("Failed to serialize init.meta.json5")?; .context("Failed to serialize init.meta.json5")?;
self.suppress_path(&meta_path); if suppress_vfs {
self.suppress_path(&meta_path);
}
fs::write(&meta_path, &content) fs::write(&meta_path, &content)
.with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?;
log::info!( log::info!(
@ -2466,7 +2440,6 @@ impl ApiService {
let meta_path = parent_dir.join(format!("{}.meta.json5", name)); let meta_path = parent_dir.join(format!("{}.meta.json5", name));
let content = let content =
crate::json::to_vec_pretty_sorted(&meta).context("Failed to serialize meta.json5")?; crate::json::to_vec_pretty_sorted(&meta).context("Failed to serialize meta.json5")?;
self.suppress_path(&meta_path);
fs::write(&meta_path, &content) fs::write(&meta_path, &content)
.with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?; .with_context(|| format!("Failed to write meta file: {}", meta_path.display()))?;
log::info!( log::info!(

29
tests/tests/two_way_sync.rs
View file

@ -12,13 +12,10 @@
//! 7. ProjectNode guard (no project file corruption) //! 7. ProjectNode guard (no project file corruption)
//! 8. Echo suppression (no redundant VFS patches) //! 8. Echo suppression (no redundant VFS patches)
//! //!
//! NOTE: Tests operate on pre-existing instances already in the Rojo tree //! NOTE: Adding new instances via the API writes files to disk WITHOUT
//! (from the initial snapshot). Adding new instances via the API writes files //! VFS echo suppression — the watcher must pick up the new files to add
//! to disk but relies on VFS events for tree updates; with echo suppression, //! them to the tree. Updates and removals of EXISTING instances DO suppress
//! those events are suppressed for the paths the API wrote. This is correct //! their VFS echoes since the tree is already mutated via handle_tree_event.
//! behavior — the plugin reconciles its own tree and doesn't need Rojo to
//! re-snapshot what it just sent. Tests that need to interact with the tree
//! (read back IDs, send updates) must use instances from the initial snapshot.
use std::collections::HashMap; use std::collections::HashMap;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
@ -721,13 +718,13 @@ fn echo_suppression_prevents_redundant_patches() {
let read_after = session.get_api_read(root_id).unwrap(); let read_after = session.get_api_read(root_id).unwrap();
let cursor_delta = read_after.message_cursor - initial_cursor; let cursor_delta = read_after.message_cursor - initial_cursor;
// With proper echo suppression, the cursor should not advance excessively. // For a single add: ~2 messages (1 from handle_tree_event broadcast +
// For a single add: 0–3 messages (the suppressed echo plus potential // 1 from VFS watcher adding the new instance to the tree). Allowing 3
// metadata/re-snapshot). Anything higher means echo events are leaking. // accounts for platform-specific directory-level VFS events.
assert!( assert!(
cursor_delta <= 3, cursor_delta <= 3,
"Echo suppression failure: cursor advanced by {} (expected <= 3). \ "Echo suppression: cursor advanced by {} (expected ~2 for add: \
VFS echo events are leaking through suppression.", 1 tree mutation + 1 VFS pickup)",
cursor_delta cursor_delta
); );
@ -2355,12 +2352,14 @@ fn echo_suppression_rapid_adds_10x() {
); );
} }
// Server should still be responsive // Server should still be responsive. 10 adds x ~2 events each
// (1 handle_tree_event + 1 VFS pickup) = ~20, with margin for
// platform-specific directory events.
let read_after = session.get_api_read(root_id).unwrap(); let read_after = session.get_api_read(root_id).unwrap();
let cursor_delta = read_after.message_cursor - initial_cursor; let cursor_delta = read_after.message_cursor - initial_cursor;
assert!( assert!(
cursor_delta < 50, cursor_delta <= 30,
"Cursor delta {} should be reasonable with echo suppression", "Cursor delta {} should be bounded (10 adds x ~2 events + margin)",
cursor_delta cursor_delta
); );