From 1a8f41f4d980e4afdf47d61381aa918f6516d79d Mon Sep 17 00:00:00 2001 From: Angus Bezzina <37071175+angusbezzina@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:12:57 -0500 Subject: [PATCH] An uncurated share's files find their way home again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `attn review share ` records a root and no curated file list. Since attn-x2zq its snapshots travel under root-relative wire paths like every other share — but `local_owner_display_path`, which turns those back into absolute paths for the owner's own window, still bailed the moment the curated list was empty. So the owner's store held `hosted.md` where the window expected `/abs/path/hosted.md`. `ownerFileIdForPath` matched nothing, the focus-following effect in App.svelte concluded the open file was unshared and called `setCurrentFile(null)`, and the review rail rendered nothing — while the comments sat correctly in the store the whole time. Reviewers' comments arrived, decrypted, and persisted, and the owner simply never saw them. attn-x2zq changed the forward direction and left the inverse behind. This mirrors it: the root is the shared directory or a shared file's own parent, membership of the curated list authorises a name when there is one, and containment in the root authorises it when there is not. Wire paths are rebuilt one plain segment at a time, so one that tries to climb out of the root resolves to nothing rather than to a file beside it. Fixing the file-share root also repairs a curated single-file share, where the old code took `record.path` — the file itself — as the root and errored out of `normalized_relative_share_path` instead of matching. Why this reached main: the native Share dialog always sends a selection, so interactive use never took the broken path, and the only coverage was the curated case. The half with no coverage is the half that shipped. The new test is mutation-checked — restoring the old bail fails it and nothing else. Verified: scripts/test-hosted-review-e2e.sh against staging goes 3 passed / 1 failed → 4 passed, and the previously failing assertion is the owner's rail rendering a browser-authored comment. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015TnncCnVNhggoZ9QLQiqeW --- src/review/bootstrap.rs | 133 ++++++++++++++++++++++++++++++++++------ 1 file changed, 114 insertions(+), 19 deletions(-) diff --git a/src/review/bootstrap.rs b/src/review/bootstrap.rs index 5152bc77..c03c8b1f 100644 --- a/src/review/bootstrap.rs +++ b/src/review/bootstrap.rs @@ -4797,29 +4797,52 @@ pub(crate) fn local_owner_display_path( let Some(record) = all.get(room_id.as_str()) else { return Ok(None); }; - if record.selected_paths.is_empty() { + // Mirror `selected_share_wire_path`, which this inverts: the root is the + // shared directory, or a shared file's own parent, and membership of the + // curated list authorises a name when there is one, containment in the + // root when there is not (attn-x2zq). + // + // Bailing on an empty curated list is what broke `attn review share + // `. Publishing had already moved to root-relative wire paths for + // those shares, so the owner's window was handed `hosted.md` where it + // expected `/abs/path/hosted.md`. `ownerFileIdForPath` then matched + // nothing, the focus-following effect concluded the open file was + // unshared and cleared the review scope, and the rail went blank while + // the comments sat in the store. + let record_root = std::path::Path::new(&record.path); + let root_for_wire = if record.is_dir { + record_root.to_path_buf() + } else { + record_root + .parent() + .map(std::path::Path::to_path_buf) + .unwrap_or_else(|| record_root.to_path_buf()) + }; + let canonical_root = root_for_wire.canonicalize().map_err(|error| { + BootstrapError::Store(format!( + "canonicalize selected share root {}: {error}", + root_for_wire.display() + )) + })?; + if !record.selected_paths.is_empty() + && !record.selected_paths.iter().any(|selected| { + normalized_relative_share_path(&canonical_root, std::path::Path::new(selected)) + .is_ok_and(|relative| relative == wire_path) + }) + { return Ok(None); } - let canonical_root = std::path::Path::new(&record.path) - .canonicalize() - .map_err(|error| { - BootstrapError::Store(format!( - "canonicalize selected share root {}: {error}", - record.path - )) - })?; - for selected in &record.selected_paths { - let selected_path = std::path::Path::new(selected); - if normalized_relative_share_path(&canonical_root, selected_path)? == wire_path { - let local_path = wire_path - .split('/') - .fold(PathBuf::from(&record.path), |path, segment| { - path.join(segment) - }); - return Ok(Some(local_path.to_string_lossy().to_string())); + // Rebuild the name one segment at a time. Only plain forward segments are + // nameable, so a wire path that tried to climb out of the root resolves to + // nothing rather than to a file beside it. + let mut local_path = root_for_wire; + for segment in wire_path.split('/') { + if segment.is_empty() || segment == "." || segment == ".." { + return Ok(None); } + local_path.push(segment); } - Ok(None) + Ok(Some(local_path.to_string_lossy().to_string())) } fn manifest_entry_for_snapshot( @@ -6301,6 +6324,78 @@ mod tests { assert_eq!(node_ref.byte_length, blob_bytes.len() as u64); } + /// `attn review share ` records no curated list, and since attn-x2zq + /// its snapshots travel under root-relative wire paths like any other + /// share. The owner's window matches those against the absolute path of + /// the file it has open, so the wire path has to survive the round trip + /// back. It did not: `local_owner_display_path` bailed whenever the + /// curated list was empty, the owner's review scope was cleared as though + /// the open file were unshared, and the review rail rendered nothing while + /// the comments sat in the store. + /// + /// The forward direction is covered by the curated test above; this is the + /// half that had no coverage, which is why the asymmetry shipped. + #[test] + fn uncurated_share_round_trips_a_wire_path_back_to_its_local_path() { + let store_tmp = TempDir::new().expect("store"); + let store_root = store_tmp.path().to_path_buf(); + std::fs::create_dir_all(shares_dir(&store_root)).expect("shares dir"); + + let project = TempDir::new().expect("project"); + let nested = project.path().join("nested"); + std::fs::create_dir(&nested).unwrap(); + std::fs::write(project.path().join("hosted.md"), "# Hosted\n").unwrap(); + std::fs::write(nested.join("child.md"), "# Child\n").unwrap(); + let canonical_project = project.path().canonicalize().expect("canonicalize project"); + + // Exactly what `attn review share ` writes: a root, no selection. + let room = "OD3XSQIjzX_KZnKC6i9ChA"; + std::fs::write( + local_shares_index_path(&store_root), + serde_json::json!({ + room: { + "path": canonical_project.to_string_lossy(), + "createdAt": 1_700_000_000_000u64, + "isDir": true, + } + }) + .to_string(), + ) + .expect("write local shares"); + let room_id = RoomId::new(room.to_string()); + + assert_eq!( + local_owner_display_path(&store_root, &room_id, "hosted.md") + .expect("local path") + .as_deref(), + Some( + canonical_project + .join("hosted.md") + .to_string_lossy() + .as_ref() + ), + "a top-level file in an uncurated share resolves to its local path" + ); + assert_eq!( + local_owner_display_path(&store_root, &room_id, "nested/child.md") + .expect("local path") + .as_deref(), + Some( + canonical_project + .join("nested") + .join("child.md") + .to_string_lossy() + .as_ref() + ), + "a nested file keeps its subdirectory" + ); + assert_eq!( + local_owner_display_path(&store_root, &room_id, "../escape.md").expect("local path"), + None, + "a wire path that climbs out of the root names nothing" + ); + } + #[tokio::test] async fn selected_share_publishes_exact_entries_manifest_with_portable_paths() { let server = MockServer::start().await;