From 640b2d3d97c20f5841e8fcd2252cc466f0a08caa Mon Sep 17 00:00:00 2001 From: Joe Date: Sun, 1 Mar 2026 07:10:14 -0600 Subject: [PATCH] Enhance git metadata handling and improve change detection logic - Introduced a new function `getGitMetadata` in `ApiContext` to fetch current git metadata, including new file IDs. - Updated the `PatchTree.build` function to handle new file IDs, allowing for better differentiation between tracked and untracked files during patch processing. - Modified the `git_changed_files` function to return both tracked and untracked files, improving accuracy in change detection. - Enhanced the `App` component to debounce git metadata refreshes, ensuring the UI reflects the latest changes without unnecessary calls. These changes aim to improve the integration with git and enhance the accuracy of change detection in the application. --- TODO.txt | 4 --- plugin/src/ApiContext.lua | 6 ++++ plugin/src/App/init.lua | 37 +++++++++++++++++--- plugin/src/PatchTree.lua | 22 +++++++++--- src/git.rs | 71 +++++++++++++++++++++++++++++---------- src/web/api.rs | 13 +++++++ src/web/interface.rs | 5 +++ 7 files changed, 128 insertions(+), 30 deletions(-) diff --git a/TODO.txt b/TODO.txt index 1ac45c66..01e57f2a 100644 --- a/TODO.txt +++ b/TODO.txt @@ -338,7 +338,3 @@ make sure this is applied to plugin-based syncback, too. [git] I notice we actually use the git cli in a lot of places. is there a git library we can use instead? - -[auto select studio/atlas] -when things get added AFTER the fact because we send the git history hashes on connect rather than continiously, it auto-selects Studio for things that are being edited in real-time in the sync dialog -how should we handle new files? because it could have been that someone just deleted it in studio. we can actually check if its a truly new file in Studio, like A but not staged! diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index 61496366..51035162 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -271,6 +271,12 @@ function ApiContext:write(patch, stageIds) end) end +function ApiContext:getGitMetadata() + local url = ("%s/api/git-metadata"):format(self.__baseUrl) + + return Http.get(url):andThen(rejectFailedRequests):andThen(Http.Response.msgpack) +end + function ApiContext:connectWebSocket(packetHandlers) local url = ("%s/api/socket/%s"):format(self.__baseUrl, self.__messageCursor) -- Convert HTTP/HTTPS URL to WS/WSS diff --git a/plugin/src/App/init.lua b/plugin/src/App/init.lua index 003042cb..fb3f693f 100644 --- a/plugin/src/App/init.lua +++ b/plugin/src/App/init.lua @@ -1000,6 +1000,9 @@ function App:startSession() return result end) + local pendingGitRefreshThread = nil + local GIT_METADATA_DEBOUNCE = 0.5 + serveSession:setPatchUpdateCallback(function(instanceMap, patch, changedIds) -- If all changes have been reverted, auto-accept the empty patch if PatchSet.isEmpty(patch) then @@ -1009,14 +1012,40 @@ function App:startSession() return end - -- Update the patchTree when new changes arrive during confirmation - -- The changedIds parameter contains IDs of items that were modified - -- so their selections can be reset (user must re-review) + -- Immediately rebuild with stale metadata so the UI reflects new items local gitMetadata = cachedServerInfo and cachedServerInfo.gitMetadata self:setState({ patchTree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }, gitMetadata), - changedIds = changedIds, -- Pass to UI so it can unselect changed items + changedIds = changedIds, }) + + -- Debounce: schedule a fresh git metadata fetch after rapid patches settle + if pendingGitRefreshThread then + task.cancel(pendingGitRefreshThread) + end + pendingGitRefreshThread = task.delay(GIT_METADATA_DEBOUNCE, function() + pendingGitRefreshThread = nil + apiContext + :getGitMetadata() + :andThen(function(freshMetadata) + if cachedServerInfo then + cachedServerInfo.gitMetadata = freshMetadata + end + if self.state.appStatus == AppStatus.Confirming then + self:setState({ + patchTree = PatchTree.build( + patch, + instanceMap, + { "Property", "Current", "Incoming" }, + freshMetadata + ), + }) + end + end) + :catch(function(err) + Log.warn("Failed to refresh git metadata: {}", tostring(err)) + end) + end) end) -- One-shot sync: disconnect after initial sync is fully complete (including any writes) diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index 9a6c5700..81bd7eeb 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -250,7 +250,7 @@ local PatchTree = {} -- Builds a new tree from a patch and instanceMap -- uses changeListHeaders in node.changeList --- gitMetadata is optional: { changedIds: {string}, scriptCommittedHashes: {[string]: {string}} } +-- gitMetadata is optional: { changedIds: {string}, scriptCommittedHashes: {[string]: {string}}, newFileIds: {string}? } function PatchTree.build(patch, instanceMap, changeListHeaders, gitMetadata) Timer.start("PatchTree.build") local clock = os.clock() @@ -267,6 +267,14 @@ function PatchTree.build(patch, instanceMap, changeListHeaders, gitMetadata) end end + local newFileIdSet = nil + if gitMetadata and gitMetadata.newFileIds then + newFileIdSet = {} + for _, id in gitMetadata.newFileIds do + newFileIdSet[id] = true + end + end + Timer.start("patch.updated") for _, change in patch.updated do clock = yieldIfNeeded(clock) @@ -518,8 +526,14 @@ function PatchTree.build(patch, instanceMap, changeListHeaders, gitMetadata) table.insert(changeList, 1, changeListHeaders) end - -- Add this node to tree - -- Default to nil (unselected) - user must explicitly choose Push/Pull/Skip + -- Truly new files (untracked in git) default to "push" (Atlas). + -- Tracked files appearing as additions are ambiguous (Studio may have + -- deleted them), so they remain nil for user review. + local defaultSelection = nil + if newFileIdSet and newFileIdSet[id] then + defaultSelection = "push" + end + tree:addNode(change.Parent, { id = change.Id, patchType = "Add", @@ -528,7 +542,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders, gitMetadata) changeInfo = changeInfo, changeList = changeList, instance = instanceMap.fromIds[id], - defaultSelection = nil, + defaultSelection = defaultSelection, }) end Timer.stop() diff --git a/src/git.rs b/src/git.rs index 24055b3a..e31a9caa 100644 --- a/src/git.rs +++ b/src/git.rs @@ -59,8 +59,23 @@ pub fn git_head_commit(repo_root: &Path) -> Option { } } -fn git_changed_files(repo_root: &Path, initial_head: Option<&str>) -> Option> { - let mut changed = HashSet::new(); +struct ChangedFiles { + tracked: HashSet, + untracked: HashSet, +} + +impl ChangedFiles { + fn all(&self) -> HashSet { + self.tracked.union(&self.untracked).cloned().collect() + } + + fn is_empty(&self) -> bool { + self.tracked.is_empty() && self.untracked.is_empty() + } +} + +fn git_changed_files(repo_root: &Path, initial_head: Option<&str>) -> Option { + let mut tracked = HashSet::new(); let diff_base = initial_head.unwrap_or("HEAD"); let diff_output = Command::new("git") @@ -78,7 +93,7 @@ fn git_changed_files(repo_root: &Path, initial_head: Option<&str>) -> Option) -> Option) -> Option = { let tree = tree_handle.lock().unwrap(); let mut result = Vec::new(); - for rel_path in &changed_files { + for rel_path in &all_changed { let abs_path = repo_root.join(rel_path); let canonical_path = std::fs::canonicalize(&abs_path).ok(); @@ -322,6 +343,11 @@ pub fn compute_git_metadata( }; // Lock released let changed_ids: Vec = resolved.iter().map(|ri| ri.id).collect(); + let new_file_ids: Vec = resolved + .iter() + .filter(|ri| untracked_set.contains(&ri.rel_path)) + .map(|ri| ri.id) + .collect(); let mut script_committed_hashes: HashMap> = HashMap::new(); #[derive(Clone, Copy)] @@ -393,6 +419,7 @@ pub fn compute_git_metadata( GitMetadata { changed_ids, script_committed_hashes, + new_file_ids, } } @@ -605,7 +632,8 @@ mod tests { fs::write(dir.path().join("script.luau"), "modified").unwrap(); let changed = git_changed_files(dir.path(), None).unwrap(); - assert!(changed.contains(&PathBuf::from("script.luau"))); + assert!(changed.tracked.contains(&PathBuf::from("script.luau"))); + assert!(changed.untracked.is_empty()); } #[test] @@ -618,7 +646,7 @@ mod tests { git_stage(dir.path(), "script.luau"); let changed = git_changed_files(dir.path(), None).unwrap(); - assert!(changed.contains(&PathBuf::from("script.luau"))); + assert!(changed.tracked.contains(&PathBuf::from("script.luau"))); } #[test] @@ -630,7 +658,8 @@ mod tests { fs::write(dir.path().join("new_file.luau"), "new").unwrap(); let changed = git_changed_files(dir.path(), None).unwrap(); - assert!(changed.contains(&PathBuf::from("new_file.luau"))); + assert!(changed.untracked.contains(&PathBuf::from("new_file.luau"))); + assert!(!changed.tracked.contains(&PathBuf::from("new_file.luau"))); } #[test] @@ -642,7 +671,7 @@ mod tests { fs::remove_file(dir.path().join("to_delete.luau")).unwrap(); let changed = git_changed_files(dir.path(), None).unwrap(); - assert!(changed.contains(&PathBuf::from("to_delete.luau"))); + assert!(changed.tracked.contains(&PathBuf::from("to_delete.luau"))); } #[test] @@ -657,9 +686,12 @@ mod tests { fs::write(dir.path().join("untracked.luau"), "brand new").unwrap(); let changed = git_changed_files(dir.path(), None).unwrap(); - assert!(changed.contains(&PathBuf::from("will_modify.luau"))); - assert!(changed.contains(&PathBuf::from("untracked.luau"))); - assert!(!changed.contains(&PathBuf::from("committed.luau"))); + let all = changed.all(); + assert!(all.contains(&PathBuf::from("will_modify.luau"))); + assert!(all.contains(&PathBuf::from("untracked.luau"))); + assert!(!all.contains(&PathBuf::from("committed.luau"))); + assert!(changed.tracked.contains(&PathBuf::from("will_modify.luau"))); + assert!(changed.untracked.contains(&PathBuf::from("untracked.luau"))); } #[test] @@ -670,7 +702,10 @@ mod tests { let changed = git_changed_files(dir.path(), None); assert!(changed.is_some()); - assert!(changed.unwrap().contains(&PathBuf::from("new.luau"))); + assert!(changed + .unwrap() + .untracked + .contains(&PathBuf::from("new.luau"))); } #[test] @@ -683,7 +718,7 @@ mod tests { fs::write(dir.path().join("src/module.luau"), "modified").unwrap(); let changed = git_changed_files(dir.path(), None).unwrap(); - assert!(changed.contains(&PathBuf::from("src/module.luau"))); + assert!(changed.tracked.contains(&PathBuf::from("src/module.luau"))); } #[test] @@ -700,13 +735,13 @@ mod tests { let without = git_changed_files(dir.path(), None).unwrap(); assert!( - !without.contains(&PathBuf::from("script.luau")), + !without.all().contains(&PathBuf::from("script.luau")), "without initial_head, committed file should NOT appear" ); let with = git_changed_files(dir.path(), Some(&initial_head)).unwrap(); assert!( - with.contains(&PathBuf::from("script.luau")), + with.tracked.contains(&PathBuf::from("script.luau")), "with initial_head, committed file should appear" ); } diff --git a/src/web/api.rs b/src/web/api.rs index 076df52d..7b77a7aa 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -160,6 +160,7 @@ pub async fn call( handle_api_syncback(request, &service, syncback_signal).await } (&Method::GET, "/api/validate-tree") => service.handle_api_validate_tree().await, + (&Method::GET, "/api/git-metadata") => service.handle_api_git_metadata().await, (_method, path) => msgpack( ErrorResponse::not_found(format!("Route not found: {}", path)), @@ -405,6 +406,18 @@ impl ApiService { }) } + async fn handle_api_git_metadata(&self) -> Response> { + let git_metadata = self.serve_session.git_repo_root().map(|repo_root| { + crate::git::compute_git_metadata( + &self.serve_session.tree_handle(), + repo_root, + self.serve_session.initial_head_commit(), + ) + }); + + msgpack_ok(&git_metadata) + } + /// Read-only tree freshness check for test infrastructure. /// Re-snapshots from disk and returns drift counts without applying corrections. async fn handle_api_validate_tree(&self) -> Response> { diff --git a/src/web/interface.rs b/src/web/interface.rs index 88244589..cd3e2647 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -214,6 +214,11 @@ pub struct GitMetadata { /// SHA1 hashes of prior versions (git blob format) for changed script instances. /// Contains 1-2 hashes per script: HEAD version and staged version (if different). pub script_committed_hashes: HashMap>, + /// Instance IDs whose backing files are untracked in git (truly new files, + /// never committed). Used by the plugin to default new-file additions to + /// "push" (Atlas) since they can't be "something Studio deleted." + #[serde(default)] + pub new_file_ids: Vec, } /// Response body from /api/rojo