diff --git a/crates/okena-daemon-core/src/pty_loop.rs b/crates/okena-daemon-core/src/pty_loop.rs index a6d11b1fb..311c46c18 100644 --- a/crates/okena-daemon-core/src/pty_loop.rs +++ b/crates/okena-daemon-core/src/pty_loop.rs @@ -1137,6 +1137,32 @@ mod tests { (repo, worktree) } + /// A shell for a hook PTY a test waits on. + /// + /// Production hooks go through `ShellType::for_command`, which runs + /// `$SHELL -ic` so the hook sees the user's aliases and environment. In a + /// test that only buys a dependency on what the developer's interactive + /// profile costs to start: an interactive zsh that takes 1.6s to reach the + /// command runs these waits out of budget on a machine where nothing is + /// wrong. What is under test is PTY exit handling, not shell resolution, so + /// run the command in a bare shell. + /// How long a test waits for a hook PTY to report its exit. + const HOOK_EXIT_BUDGET: Duration = Duration::from_secs(20); + + fn hook_shell(command: &str) -> ShellType { + if cfg!(windows) { + ShellType::Custom { + path: "cmd".to_string(), + args: vec!["/C".to_string(), command.to_string()], + } + } else { + ShellType::Custom { + path: "/bin/sh".to_string(), + args: vec!["-c".to_string(), command.to_string()], + } + } + } + fn workspace_with_pending_close( main_repo: &Path, worktree: &Path, @@ -1744,7 +1770,7 @@ mod tests { let hook_terminal_id = pty_manager .create_terminal_with_shell( worktree.to_str().expect("utf-8 worktree path"), - Some(&ShellType::for_command("exit 0".to_string())), + Some(&hook_shell("exit 0")), ) .expect("create before-remove hook PTY"); let pty_manager = Arc::new(pty_manager); @@ -1782,7 +1808,9 @@ mod tests { let mut exit_events = Vec::new(); let mut dirty_terminal_ids = Vec::new(); let mut budget = TurnBudget::default(); - tokio::time::timeout(Duration::from_secs(2), async { + // Generous on purpose: this budget is here to turn a hang + // into a failure, not to assert how fast a PTY starts. + tokio::time::timeout(HOOK_EXIT_BUDGET, async { while exit_events.is_empty() { let event = pty_events.recv().await.expect("receive hook PTY event"); process_event( @@ -1868,7 +1896,7 @@ mod tests { let hook_terminal_id = pty_manager .create_terminal_with_shell( worktree.to_str().expect("utf-8 worktree path"), - Some(&ShellType::for_command("exit 0".to_string())), + Some(&hook_shell("exit 0")), ) .expect("create before-remove hook PTY"); let pty_manager = Arc::new(pty_manager); @@ -1907,7 +1935,9 @@ mod tests { let mut exit_events = Vec::new(); let mut dirty_terminal_ids = Vec::new(); let mut budget = TurnBudget::default(); - tokio::time::timeout(Duration::from_secs(2), async { + // Generous on purpose: this budget is here to turn a hang + // into a failure, not to assert how fast a PTY starts. + tokio::time::timeout(HOOK_EXIT_BUDGET, async { while exit_events.is_empty() { let event = pty_events.recv().await.expect("receive hook PTY event"); process_event( @@ -1978,7 +2008,7 @@ mod tests { let hook_terminal_id = pty_manager .create_terminal_with_shell( worktree.to_str().expect("utf-8 worktree path"), - Some(&ShellType::for_command("sleep 30".to_string())), + Some(&hook_shell("sleep 30")), ) .expect("create keep-alive before-remove hook PTY"); let pty_manager = Arc::new(pty_manager); diff --git a/crates/okena-git/src/error.rs b/crates/okena-git/src/error.rs index 36de7a24a..c0c2fbedd 100644 --- a/crates/okena-git/src/error.rs +++ b/crates/okena-git/src/error.rs @@ -19,8 +19,11 @@ pub enum GitError { #[error("directory '{path}' is already an active worktree")] WorktreeExists { path: PathBuf }, - /// Failed to remove a directory. - #[error("failed to remove directory '{path}'")] + /// Failed to remove a directory. The cause belongs in the message: this + /// error reaches the user as a toast and the log as `{e}`, and neither + /// walks the source chain, so without it a failed worktree close reports + /// only the path it already named. + #[error("failed to remove directory '{path}': {source}")] RemoveFailed { path: PathBuf, #[source] @@ -100,6 +103,20 @@ mod tests { ); } + /// The io cause is what says whether the close failed on a busy directory, + /// a permission, or a vanished path. Dropping it leaves the user with a + /// sentence that only repeats the path. + #[test] + fn a_failed_removal_names_its_cause() { + let err = GitError::RemoveFailed { + path: PathBuf::from("/tmp/wt"), + source: std::io::Error::from(std::io::ErrorKind::DirectoryNotEmpty), + }; + let message = err.user_detail(); + assert!(message.contains("/tmp/wt"), "{message}"); + assert!(message.contains("not empty"), "{message}"); + } + #[test] fn the_last_failure_line_wins() { let err = GitError::GitExitError { diff --git a/crates/okena-git/src/repository/ci.rs b/crates/okena-git/src/repository/ci.rs index e31f431a3..fc5aff3ba 100644 --- a/crates/okena-git/src/repository/ci.rs +++ b/crates/okena-git/src/repository/ci.rs @@ -13,7 +13,7 @@ use okena_core::process::{command, safe_output_with_timeout}; use serde_json::{Value, json}; use super::github::{ApiError, GithubClient, GithubRepo, resolve_base_repo}; -use super::status::get_pushed_sha; +use super::status::get_upstream_ref; /// Hard cap on the remaining `gh` invocation. `gh` can hang indefinitely — /// auth prompts or a stalled network — and the bus kills the process when @@ -135,6 +135,33 @@ struct PrNode { head_ref_oid: Option, } +/// The pushed commit that belongs to *this* branch, or `None` when the branch +/// tracks somebody else's. +/// +/// Git records an upstream whenever a branch starts from a remote-tracking ref, +/// and a worktree branch starts from `origin/`. Until it is pushed, +/// its upstream is therefore the default branch, and a lookup keyed on that +/// commit answers with the default branch's CI: a nightly deploy reported as a +/// failure against a worktree that never triggered it, and a PR head compared +/// against a commit from another branch. +/// +/// A branch that deliberately tracks a differently named remote branch still +/// gets its own answer. Only the default branch is treated as a base rather +/// than a counterpart, which is the case Okena creates itself. +fn branch_pushed_sha(path: &Path) -> Option { + let upstream = get_upstream_ref(path)?; + let branch = super::status::get_current_branch(path)?; + if upstream.branch == branch { + return Some(upstream.sha); + } + // Only reached for the mismatch, so the default-branch lookup stays off the + // path every well-tracked branch takes. + match super::branch::get_default_branch(path) { + Some(default) if default == upstream.branch => None, + _ => Some(upstream.sha), + } +} + /// Get PR info for the current branch (if any PR exists). /// /// Matches by head branch name in the base repository, like `gh pr list @@ -146,7 +173,7 @@ pub fn fetch_pr_info(path: &Path) -> PrFetch { return PrFetch::Fetched(None); }; let current_sha = super::status::get_head_sha(path); - let pushed_sha = get_pushed_sha(path); + let pushed_sha = branch_pushed_sha(path); let Some((mut client, repo)) = github_client(path) else { return PrFetch::Fetched(None); }; @@ -340,9 +367,9 @@ fn rollup_status(failed: usize, pending: usize) -> crate::CiStatus { /// /// With a known PR number, reads the PR's status-check rollup (Actions + /// external status checks aggregated by the PR, as `gh pr checks` does). -/// Otherwise falls back to `check-runs` + `status` on the current upstream -/// commit, which works for any pushed branch — including default branches -/// without a PR. +/// Otherwise falls back to `check-runs` + `status` on the branch's own pushed +/// commit (see `branch_pushed_sha`), which works for any pushed branch, +/// default branches without a PR included. /// /// `unchanged_sha` is the upstream commit a *settled* cached summary describes. /// Checks on a given commit only move while something is running, so when the @@ -358,7 +385,7 @@ pub fn fetch_ci_checks( unchanged_sha: Option<&str>, ) -> CiFetch { // Read locally (gix, no network) before deciding to spend a request. - let sha = get_pushed_sha(path); + let sha = branch_pushed_sha(path); if let (Some(sha), Some(cached)) = (sha.as_deref(), unchanged_sha) && sha == cached { @@ -1247,13 +1274,65 @@ mod tests { ); super::super::test_support::git_in(&repo, &["push", "-u", "origin", "main"]); - let sha = super::get_pushed_sha(&repo).expect("branch has an upstream"); + let sha = super::super::status::get_pushed_sha(&repo).expect("branch has an upstream"); assert_eq!( super::fetch_ci_checks(&repo, None, Some(&sha)), super::CiFetch::Unchanged ); } + /// The exact shape a worktree branch had before `--no-track`: it tracks the + /// default branch, so its "upstream commit" is main's tip. A lookup keyed + /// on that commit answers with main's CI, which is how a nightly deploy + /// failure ended up reported against a feature worktree. + #[test] + fn a_branch_tracking_the_default_branch_is_not_asked_about() { + let (_tmp, repo, _remote) = super::super::test_support::repo_with_origin(); + super::super::test_support::git_in(&repo, &["checkout", "-q", "-b", "feat/x"]); + // What `git worktree add -b feat/x origin/main` used to record. + super::super::test_support::git_in(&repo, &["config", "branch.feat/x.remote", "origin"]); + super::super::test_support::git_in( + &repo, + &["config", "branch.feat/x.merge", "refs/heads/main"], + ); + + assert!( + super::branch_pushed_sha(&repo).is_none(), + "main's tip is not this branch's pushed commit" + ); + // And no request is spent finding that out. + assert_eq!( + super::fetch_ci_checks(&repo, None, None), + super::CiFetch::Fetched { + sha: None, + summary: None + } + ); + } + + /// A branch that tracks its own counterpart still gets its own answer, and + /// so does one deliberately tracking a differently named remote branch. + #[test] + fn a_branch_tracking_its_own_remote_keeps_its_commit() { + let (_tmp, repo, _remote) = super::super::test_support::repo_with_origin(); + super::super::test_support::git_in(&repo, &["checkout", "-q", "-b", "feat/x"]); + super::super::test_support::git_in(&repo, &["push", "-q", "-u", "origin", "feat/x"]); + let own = super::branch_pushed_sha(&repo).expect("its own upstream counts"); + + // Now point it at a non-default remote branch under another name. + super::super::test_support::git_in(&repo, &["push", "-q", "origin", "feat/x:other"]); + super::super::test_support::git_in(&repo, &["fetch", "-q", "origin"]); + super::super::test_support::git_in( + &repo, + &["config", "branch.feat/x.merge", "refs/heads/other"], + ); + assert_eq!( + super::branch_pushed_sha(&repo).as_deref(), + Some(own.as_str()), + "tracking another branch on purpose is still this branch's answer" + ); + } + #[test] fn branch_without_upstream_never_reaches_the_api() { // Nothing is pushed, so there is nothing CI could have run on. diff --git a/crates/okena-git/src/repository/mod.rs b/crates/okena-git/src/repository/mod.rs index 1bb9692db..876a360d7 100644 --- a/crates/okena-git/src/repository/mod.rs +++ b/crates/okena-git/src/repository/mod.rs @@ -166,6 +166,18 @@ pub(crate) mod test_support { String::from_utf8_lossy(&status.stderr) ); } + /// A repo whose `main` is pushed to a bare `origin`. The second tempdir owns + /// the remote and must stay alive for the repo's lifetime. + pub(crate) fn repo_with_origin() -> (tempfile::TempDir, PathBuf, tempfile::TempDir) { + let (tmp, repo) = init_temp_repo(); + let remote_tmp = tempfile::tempdir().expect("create remote tempdir"); + let remote = remote_tmp.path().join("remote.git"); + let remote_str = remote.to_str().expect("remote path is utf-8"); + git_in(&repo, &["init", "--bare", "-b", "main", remote_str]); + git_in(&repo, &["remote", "add", "origin", remote_str]); + git_in(&repo, &["push", "-q", "origin", "main"]); + (tmp, repo, remote_tmp) + } } #[cfg(test)] diff --git a/crates/okena-git/src/repository/status.rs b/crates/okena-git/src/repository/status.rs index b5f7c52af..048076530 100644 --- a/crates/okena-git/src/repository/status.rs +++ b/crates/okena-git/src/repository/status.rs @@ -197,14 +197,23 @@ pub fn get_head_sha(path: &Path) -> Option { Some(id.to_hex().to_string()) } -/// Full SHA of the current branch's upstream tracking commit — the last commit -/// known (from the latest fetch) to be on the remote. `None` if HEAD is -/// detached or the branch has no upstream (never pushed). +/// The remote branch the current branch tracks, and that branch's commit. /// -/// Branch-level CI lookups (`/commits/{sha}/check-runs` and `/status`) must use -/// this rather than the local HEAD: GitHub runs CI against *pushed* commits, so -/// querying an unpushed local HEAD just returns nothing. -pub fn get_pushed_sha(path: &Path) -> Option { +/// The name matters as much as the commit: a branch does not necessarily track +/// its own counterpart. Git records an upstream whenever a branch is started +/// from a remote-tracking ref, so a branch created from `origin/main` tracks +/// `main` until it is pushed, and its "upstream commit" is the default +/// branch's tip rather than anything this branch did. +pub struct UpstreamRef { + /// Branch name on the remote, without the remote prefix: `main`, `feat/x`. + pub branch: String, + /// Full SHA that remote branch points at, as of the latest fetch. + pub sha: String, +} + +/// The current branch's upstream. `None` if HEAD is detached or the branch has +/// no upstream (never pushed, and not started from a remote-tracking ref). +pub fn get_upstream_ref(path: &Path) -> Option { let repo = crate::gix_helpers::open(path)?; let branch = super::head_branch_short(&repo)?; let head_ref = repo @@ -218,7 +227,29 @@ pub fn get_pushed_sha(path: &Path) -> Option { .rev_parse_single(upstream_name.as_bstr()) .ok()? .detach(); - Some(id.to_hex().to_string()) + Some(UpstreamRef { + branch: remote_branch_name(&upstream_name.as_bstr().to_string())?, + sha: id.to_hex().to_string(), + }) +} + +/// `refs/remotes/origin/feat/x` -> `feat/x`. The remote is one segment, the +/// branch is everything after it, slashes included. +fn remote_branch_name(full_ref: &str) -> Option { + let (_remote, branch) = full_ref.strip_prefix("refs/remotes/")?.split_once('/')?; + (!branch.is_empty()).then(|| branch.to_string()) +} + +/// Full SHA of the current branch's upstream tracking commit: the last commit +/// known (from the latest fetch) to be on the remote. `None` if HEAD is +/// detached or the branch has no upstream (never pushed). +/// +/// Branch-level CI lookups (`/commits/{sha}/check-runs` and `/status`) must use +/// this rather than the local HEAD: GitHub runs CI against *pushed* commits, so +/// querying an unpushed local HEAD just returns nothing. They must also check +/// *whose* upstream it is (see `UpstreamRef`). +pub fn get_pushed_sha(path: &Path) -> Option { + get_upstream_ref(path).map(|upstream| upstream.sha) } /// Tracked per-file diff counts and the untracked-file list, produced by a @@ -665,6 +696,21 @@ mod tests { assert!(get_current_branch(&path).is_none()); } + #[test] + fn a_remote_ref_splits_into_remote_and_branch() { + assert_eq!( + remote_branch_name("refs/remotes/origin/main").as_deref(), + Some("main") + ); + // A slash in the branch name belongs to the branch, not the remote. + assert_eq!( + remote_branch_name("refs/remotes/origin/feat/x").as_deref(), + Some("feat/x") + ); + assert_eq!(remote_branch_name("refs/heads/main"), None); + assert_eq!(remote_branch_name("refs/remotes/origin/"), None); + } + #[test] fn count_unpushed_commits_returns_none_for_invalid_path() { let path = PathBuf::from("/nonexistent/path/that/does/not/exist"); diff --git a/crates/okena-git/src/repository/worktree.rs b/crates/okena-git/src/repository/worktree.rs index 4a87d4015..4ea85f328 100644 --- a/crates/okena-git/src/repository/worktree.rs +++ b/crates/okena-git/src/repository/worktree.rs @@ -1,6 +1,7 @@ //! Worktree operations: create / remove / list. use std::path::{Path, PathBuf}; +use std::time::Duration; use okena_core::process::{command, safe_output}; @@ -462,6 +463,17 @@ pub fn create_worktree( let mut args = vec!["-C", repo_str, "worktree", "add"]; match &attachment { BranchAttachment::NewBranch(start_point) => { + // The start point is `origin/`, and git records a + // remote-tracking start point as the new branch's upstream + // (`branch.autoSetupMerge`). Nothing has been pushed yet, so that + // upstream would be the default branch: every lookup for "this + // branch's pushed commit" would answer with the default branch's, + // reporting its CI against a worktree that never triggered it. + // `git push -u` records the real one when there is something to + // record. + if start_point.is_some() { + args.push("--no-track"); + } args.push("-b"); args.push(branch); args.push(target_str); @@ -505,9 +517,15 @@ pub fn create_worktree_with_start_point( let repo_str = path_str(repo_path)?; let target_str = path_str(target_path)?; - let mut args = vec!["-C", repo_str, "worktree", "add", "-b", branch, target_str]; - let start_point = start_branch.and_then(|sb| resolve_start_ref(repo_path, sb)); + + let mut args = vec!["-C", repo_str, "worktree", "add"]; + // See `create_worktree`: a branch that has never been pushed must not claim + // the branch it started from as its upstream. + if start_point.is_some() { + args.push("--no-track"); + } + args.extend(["-b", branch, target_str]); if let Some(start_point) = &start_point { args.push(start_point); } @@ -586,7 +604,7 @@ pub fn remove_worktree_fast(verified: &VerifiedWorktree) -> GitResult<()> { fn remove_worktree_fast_with( verified: &VerifiedWorktree, - remove_dir_all: impl FnOnce(&Path) -> std::io::Result<()>, + remove_dir_all: impl FnMut(&Path) -> std::io::Result<()>, ) -> GitResult<()> { revalidate_verified_worktree(verified)?; quarantine_and_delete( @@ -597,6 +615,87 @@ fn remove_worktree_fast_with( ) } +/// Prefix of the hidden directory a checkout is renamed to before deletion. +const QUARANTINE_PREFIX: &str = ".okena-removing-"; + +/// How many times a delete is attempted before the failure stands, and how long +/// to wait between attempts. +const DELETE_ATTEMPTS: usize = 4; +const DELETE_RETRY_DELAY: Duration = Duration::from_millis(150); + +/// Whether a failed delete is worth another attempt. +/// +/// `remove_dir_all` walks the tree and then removes the directory itself, so +/// anything that writes into the checkout during that walk (a watcher that +/// outlived the shell it was started from, Spotlight, Finder) leaves the final +/// `rmdir` reporting a directory that is not empty, with nothing actually +/// wrong. A permission error is not a race: it fails the same way every time, +/// and retrying only delays the report. +fn is_transient_delete_error(error: &std::io::Error) -> bool { + matches!( + error.kind(), + std::io::ErrorKind::DirectoryNotEmpty + | std::io::ErrorKind::ResourceBusy + | std::io::ErrorKind::Interrupted + ) +} + +/// Delete a directory, retrying while the failure looks like a race with +/// something still writing into it. An already absent directory is a success. +fn delete_with_retries( + path: &Path, + mut remove_dir_all: impl FnMut(&Path) -> std::io::Result<()>, +) -> std::io::Result<()> { + let mut attempt = 1; + loop { + match remove_dir_all(path) { + Ok(()) => return Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => { + if attempt >= DELETE_ATTEMPTS || !is_transient_delete_error(&error) { + return Err(error); + } + log::info!( + "worktree removal: delete of '{}' hit a transient failure ({error}), attempt {attempt} of {DELETE_ATTEMPTS}", + path.display() + ); + std::thread::sleep(DELETE_RETRY_DELAY); + attempt += 1; + } + } + } +} + +/// What a half-finished deletion left behind, as a clause to append to the +/// failure. Empty when the checkout is whole, or when git cannot say. +/// +/// A delete removes as it walks, so a refusal part-way leaves the checkout +/// short of whatever went before it. Restoring the directory to its old path +/// makes the project openable again, but "the checkout remains" is only half +/// true, and the user has no reason to suspect the rest. +fn partial_checkout_note(worktree_path: &Path) -> String { + let Ok(path) = path_str(worktree_path) else { + return String::new(); + }; + let Ok(output) = safe_output(command("git").args(["-C", path, "status", "--porcelain"])) else { + return String::new(); + }; + if !output.status.success() { + return String::new(); + } + let deleted = String::from_utf8_lossy(&output.stdout) + .lines() + .filter(|line| line.starts_with(" D") || line.starts_with("D ")) + .count(); + if deleted == 0 { + return String::new(); + } + format!( + "; the deletion had already removed {deleted} tracked file(s) before it failed, \ + `git restore .` in the checkout puts them back (untracked files it removed are gone)" + ) +} + /// Rename the checkout aside, re-prove it is still the directory whose /// `identity` was verified, delete it, then prune the parent's stale worktree /// metadata. Shared by the verified and orphaned removal paths so both get the @@ -606,12 +705,12 @@ fn quarantine_and_delete( worktree_path: &Path, identity: &FilesystemObjectIdentity, parent_path: &Path, - remove_dir_all: impl FnOnce(&Path) -> std::io::Result<()>, + remove_dir_all: impl FnMut(&Path) -> std::io::Result<()>, ) -> GitResult<()> { let parent = worktree_path .parent() .ok_or_else(|| unsafe_worktree(worktree_path, "checkout directory has no parent"))?; - let quarantine = parent.join(format!(".okena-removing-{}", uuid::Uuid::new_v4())); + let quarantine = parent.join(format!("{QUARANTINE_PREFIX}{}", uuid::Uuid::new_v4())); std::fs::rename(worktree_path, &quarantine).map_err(|source| GitError::RemoveFailed { path: worktree_path.to_path_buf(), source, @@ -637,17 +736,21 @@ fn quarantine_and_delete( return Err(unsafe_worktree(worktree_path, reason)); } - match remove_dir_all(&quarantine) { + match delete_with_retries(&quarantine, remove_dir_all) { Ok(()) => {} - Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} Err(error) => { // `remove_dir_all` can have already removed the checkout and leave // only Finder metadata behind. Delete that narrow, verified class of // debris; otherwise restore the still-owned quarantine and fail closed. if let Err(cleanup_error) = cleanup_benign_residual(&quarantine) { + log::warn!( + "worktree removal: quarantine at '{}' still holds the checkout: {cleanup_error}", + quarantine.display() + ); let source = match std::fs::rename(&quarantine, worktree_path) { Ok(()) => std::io::Error::other(format!( - "{error}; residual cleanup refused: {cleanup_error}" + "{error}{}", + partial_checkout_note(worktree_path) )), Err(restore_error) => std::io::Error::new( error.kind(), @@ -775,7 +878,7 @@ fn path_identity(path: &Path) -> PathBuf { #[cfg(test)] mod tests { use super::*; - use crate::repository::test_support::{git_in, init_temp_repo}; + use crate::repository::test_support::{git_in, init_temp_repo, repo_with_origin}; use std::path::PathBuf; #[test] @@ -941,6 +1044,177 @@ mod tests { ); } + /// A branch a worktree was just created on has never been pushed, so it has + /// no upstream. Git sets one when the start point is a remote-tracking ref, + /// and the start point here is `origin/`: the new branch would + /// claim the default branch as its upstream, and every lookup for "this + /// branch's pushed commit" would answer with the default branch's tip. + #[test] + fn a_new_worktree_branch_claims_no_upstream() { + let (_tmp, repo, _remote) = repo_with_origin(); + let target_parent = tempfile::tempdir().expect("create target parent"); + let target = target_parent.path().join("wt-feat"); + + create_worktree(&repo, "feat/x", &target, true).expect("create worktree"); + + assert_eq!( + git_out(&target, &["symbolic-ref", "--short", "HEAD"]), + "feat/x" + ); + assert_eq!( + git_out( + &repo, + &["config", "--default", "", "--get", "branch.feat/x.merge"] + ), + "", + "a branch with nothing pushed must not track the branch it started from" + ); + } + + /// Same for the pre-resolved start point path, which skips the fetch. + #[test] + fn a_worktree_created_from_a_start_point_claims_no_upstream() { + let (_tmp, repo, _remote) = repo_with_origin(); + let target_parent = tempfile::tempdir().expect("create target parent"); + let target = target_parent.path().join("wt-feat"); + + create_worktree_with_start_point(&repo, "feat/y", &target, Some("main")) + .expect("create worktree"); + + assert_eq!( + git_out(&target, &["symbolic-ref", "--short", "HEAD"]), + "feat/y" + ); + assert_eq!( + git_out( + &repo, + &["config", "--default", "", "--get", "branch.feat/y.merge"] + ), + "" + ); + } + + #[cfg(unix)] + #[test] + fn fast_removal_does_not_follow_directory_symlinks() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let root = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", root.to_str().unwrap(), "-b", "feat"], + ); + std::fs::create_dir_all(root.join("nested")).expect("create tree"); + std::fs::write(root.join("nested").join("file.txt"), "x").expect("write file"); + let outside = wt_tmp.path().join("outside"); + std::fs::create_dir(&outside).expect("create outside directory"); + std::fs::write(outside.join("keep.txt"), "must survive").expect("write outside file"); + std::os::unix::fs::symlink(&outside, root.join("link")).expect("create symlink"); + + let verified = verify_linked_worktree_fresh(&repo, &root).expect("verify worktree"); + remove_worktree_fast(&verified).expect("remove the worktree"); + + assert!(!root.exists()); + assert_eq!( + std::fs::read_to_string(outside.join("keep.txt")).unwrap(), + "must survive" + ); + } + + /// A checkout is deleted while the machine keeps running, so a watcher or + /// an indexer can drop a file into the tree between the walk and the final + /// `rmdir`. That is a race, not a verdict: the removal used to report the + /// whole close as failed and put the checkout back. + #[test] + fn a_delete_losing_a_race_is_retried() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let wt_path = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], + ); + let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); + + let attempts = std::cell::Cell::new(0usize); + let result = remove_worktree_fast_with(&verified, |quarantine| { + attempts.set(attempts.get() + 1); + if attempts.get() == 1 { + return Err(std::io::Error::from(std::io::ErrorKind::DirectoryNotEmpty)); + } + std::fs::remove_dir_all(quarantine) + }); + + assert!(result.is_ok(), "{result:?}"); + assert_eq!(attempts.get(), 2, "the second attempt should have run"); + assert!(!wt_path.exists(), "the checkout is gone"); + assert!( + std::fs::read_dir(wt_tmp.path()).unwrap().next().is_none(), + "no quarantine is left behind" + ); + } + + /// A permission failure repeats identically however often it is tried, and + /// the cause has to survive into the message: that sentence is the whole of + /// what the user is told when a close fails. + #[test] + fn a_permission_failure_is_reported_once_with_its_cause() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let wt_path = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], + ); + let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); + + let attempts = std::cell::Cell::new(0usize); + let result = remove_worktree_fast_with(&verified, |_| { + attempts.set(attempts.get() + 1); + Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied)) + }); + + let error = result.expect_err("a permission failure must not be swallowed"); + assert_eq!(attempts.get(), 1, "retrying only delays the same failure"); + assert!( + error.to_string().contains("permission denied"), + "the cause must reach the message: {error}" + ); + assert!(wt_path.exists(), "the checkout is restored, not lost"); + } + + #[test] + fn removal_preserves_preexisting_quarantines() { + let (_tmp, repo) = init_temp_repo(); + let wt_tmp = tempfile::tempdir().expect("create worktree tempdir"); + let abandoned = wt_tmp + .path() + .join(format!("{QUARANTINE_PREFIX}{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(abandoned.join("src")).expect("create abandoned quarantine"); + std::fs::write(abandoned.join("src").join("main.rs"), "preserved checkout") + .expect("fill abandoned quarantine"); + let foreign = wt_tmp.path().join(format!("{QUARANTINE_PREFIX}not-a-uuid")); + std::fs::create_dir(&foreign).expect("create lookalike directory"); + + let wt_path = wt_tmp.path().join("wt-feat"); + git_in( + &repo, + &["worktree", "add", wt_path.to_str().unwrap(), "-b", "feat"], + ); + let verified = verify_linked_worktree_fresh(&repo, &wt_path).expect("verify worktree"); + remove_worktree_fast(&verified).expect("remove worktree"); + + assert!(!wt_path.exists(), "the requested checkout is removed"); + assert_eq!( + std::fs::read_to_string(abandoned.join("src/main.rs")).unwrap(), + "preserved checkout" + ); + assert!( + foreign.exists(), + "a name this module never wrote is not ours" + ); + } + #[test] fn guarded_fast_removal_rejects_a_replaced_checkout() { let (_tmp, repo) = init_temp_repo(); @@ -1127,19 +1401,6 @@ mod tests { git_in(repo, &["-c", "commit.gpgsign=false", "commit", "-m", name]); } - /// A repo whose `main` is pushed to a bare `origin`. The second tempdir owns - /// the remote and must stay alive for the repo's lifetime. - fn repo_with_origin() -> (tempfile::TempDir, PathBuf, tempfile::TempDir) { - let (tmp, repo) = init_temp_repo(); - let remote_tmp = tempfile::tempdir().expect("create remote tempdir"); - let remote = remote_tmp.path().join("remote.git"); - let remote_str = remote.to_str().expect("remote path is utf-8"); - git_in(&repo, &["init", "--bare", "-b", "main", remote_str]); - git_in(&repo, &["remote", "add", "origin", remote_str]); - git_in(&repo, &["push", "-q", "origin", "main"]); - (tmp, repo, remote_tmp) - } - /// Push `feature` and drop the local copy, leaving only `origin/feature` — /// the shape a remote-only entry in the branch picker has. fn push_and_forget_feature(repo: &Path) { diff --git a/crates/okena-markdown/src/lib.rs b/crates/okena-markdown/src/lib.rs index 6ffe469ff..eae31ecb4 100644 --- a/crates/okena-markdown/src/lib.rs +++ b/crates/okena-markdown/src/lib.rs @@ -119,12 +119,19 @@ impl MarkdownDocument { /// colour, exactly as before. pub fn highlight_code_blocks(&mut self, is_dark: bool) { for node in &mut self.nodes { - if let Node::CodeBlock { + Self::highlight_node(node, is_dark); + } + } + + /// Walk into list items too: a fenced block inside a numbered step is a + /// block of the item, not a top-level node. + fn highlight_node(node: &mut Node, is_dark: bool) { + match node { + Node::CodeBlock { language, code, highlighted, - } = node - { + } => { *highlighted = okena_highlight::syntax::highlight_code_block( code, language.as_deref(), @@ -134,6 +141,14 @@ impl MarkdownDocument { .map(|line| line.spans) .collect(); } + Node::List { items, .. } => { + for item in items { + for block in &mut item.blocks { + Self::highlight_node(block, is_dark); + } + } + } + _ => {} } } } diff --git a/crates/okena-markdown/src/parser.rs b/crates/okena-markdown/src/parser.rs index c2e27b540..d1399e252 100644 --- a/crates/okena-markdown/src/parser.rs +++ b/crates/okena-markdown/src/parser.rs @@ -3,7 +3,75 @@ use pulldown_cmark::{CodeBlockKind, Event, HeadingLevel, Options, Parser, Tag, TagEnd}; use super::MarkdownDocument; -use super::types::{FmValue, Frontmatter, Inline, Node}; +use super::types::{FmValue, Frontmatter, Inline, ListItem, Node}; + +/// A block container that is currently open. +/// +/// Lists nest (a list inside an item inside a list), so the parser keeps them +/// on a stack. The flat `in_list` / `list_items` state this replaced could only +/// describe one list at a time: a nested list cleared the outer list's items, +/// took its ordered-ness, and closed it early, which dropped every item after +/// the nesting point out of the list entirely. +enum Frame { + List { + ordered: bool, + start: u64, + items: Vec, + }, + Item { + blocks: Vec, + }, + Blockquote { + blocks: Vec, + }, +} + +/// Route a finished block to the innermost open block container (a list item or +/// a quote), or to the document root when none is open. +fn push_block(nodes: &mut Vec, frames: &mut [Frame], node: Node) { + match frames.last_mut() { + Some(Frame::Item { blocks } | Frame::Blockquote { blocks }) => blocks.push(node), + _ => nodes.push(node), + } +} + +/// Turn the inline text collected directly under the innermost item into a +/// paragraph block. +/// +/// A *tight* list item (no blank line between items) carries its text as bare +/// inline events with no `Paragraph` around it, so the text has to be closed off +/// by hand: before any block opens inside the item, and when the item ends. +fn flush_item_inlines(inline_stack: &mut [Vec], frames: &mut [Frame]) { + let Some(Frame::Item { blocks }) = frames.last_mut() else { + return; + }; + let Some(pending) = inline_stack.last_mut() else { + return; + }; + if pending.is_empty() { + return; + } + blocks.push(Node::Paragraph { + children: std::mem::take(pending), + }); +} + +/// Whether an event opens or closes a block inside a list item, and so has to +/// be preceded by [`flush_item_inlines`]. +fn is_item_block_boundary(event: &Event) -> bool { + matches!( + event, + Event::Start( + Tag::Paragraph + | Tag::Heading { .. } + | Tag::CodeBlock(_) + | Tag::List(_) + | Tag::BlockQuote(_) + | Tag::Table(_) + ) | Event::End(TagEnd::Item) + | Event::Rule + ) +} /// Append `text` to the innermost inline run, merging it into the preceding text /// rather than starting a new one. The renderer lays each run out as its own @@ -54,10 +122,8 @@ impl MarkdownDocument { let mut in_code_block = false; let mut code_block_lang: Option = None; let mut code_block_content = String::new(); - let mut in_list = false; - let mut list_ordered = false; - let mut list_items: Vec> = Vec::new(); - let mut in_blockquote = false; + // Open block containers (lists, items, quotes), innermost last. + let mut frames: Vec = Vec::new(); let mut in_table = false; let mut in_table_head = false; let mut table_headers: Vec> = Vec::new(); @@ -65,6 +131,9 @@ impl MarkdownDocument { let mut current_row: Vec> = Vec::new(); for event in parser { + if is_item_block_boundary(&event) { + flush_item_inlines(&mut inline_stack, &mut frames); + } match event { // Block elements Event::Start(Tag::Heading { level, .. }) => { @@ -81,7 +150,7 @@ impl MarkdownDocument { Event::End(TagEnd::Heading(_)) => { if let Some(level) = in_heading.take() { let children = inline_stack.pop().unwrap_or_default(); - nodes.push(Node::Heading { level, children }); + push_block(&mut nodes, &mut frames, Node::Heading { level, children }); } } Event::Start(Tag::Paragraph) => { @@ -90,23 +159,13 @@ impl MarkdownDocument { } Event::End(TagEnd::Paragraph) if in_paragraph => { let children = inline_stack.pop().unwrap_or_default(); - if in_blockquote { - // Add to blockquote - if let Some(last) = inline_stack.last_mut() { - last.extend(children); - } - } else if in_list { - // Will be collected by Item end - if let Some(last) = inline_stack.last_mut() { - last.extend(children); - } - } else if in_table { - // Table cell content + if in_table { + // Collected by the table-cell end instead. if let Some(last) = inline_stack.last_mut() { last.extend(children); } } else { - nodes.push(Node::Paragraph { children }); + push_block(&mut nodes, &mut frames, Node::Paragraph { children }); } in_paragraph = false; } @@ -119,43 +178,67 @@ impl MarkdownDocument { code_block_content.clear(); } Event::End(TagEnd::CodeBlock) => { - nodes.push(Node::CodeBlock { - language: code_block_lang.take(), - code: std::mem::take(&mut code_block_content), - highlighted: Vec::new(), - }); + push_block( + &mut nodes, + &mut frames, + Node::CodeBlock { + language: code_block_lang.take(), + code: std::mem::take(&mut code_block_content), + highlighted: Vec::new(), + }, + ); in_code_block = false; } Event::Start(Tag::List(first_item)) => { - in_list = true; - list_ordered = first_item.is_some(); - list_items.clear(); + frames.push(Frame::List { + ordered: first_item.is_some(), + start: first_item.unwrap_or(1), + items: Vec::new(), + }); } Event::End(TagEnd::List(_)) => { - nodes.push(Node::List { - ordered: list_ordered, - items: std::mem::take(&mut list_items), - }); - in_list = false; + if let Some(Frame::List { + ordered, + start, + items, + }) = frames.pop() + { + push_block( + &mut nodes, + &mut frames, + Node::List { + ordered, + start, + items, + }, + ); + } } Event::Start(Tag::Item) => { + frames.push(Frame::Item { blocks: Vec::new() }); + // Holds text written straight into the item (a tight list); + // `flush_item_inlines` turns it into a paragraph block. inline_stack.push(Vec::new()); } Event::End(TagEnd::Item) => { - let children = inline_stack.pop().unwrap_or_default(); - list_items.push(children); + // Emptied by the flush that ran for this event. + inline_stack.pop(); + if let Some(Frame::Item { blocks }) = frames.pop() + && let Some(Frame::List { items, .. }) = frames.last_mut() + { + items.push(ListItem { blocks }); + } } Event::Start(Tag::BlockQuote(_)) => { - in_blockquote = true; - inline_stack.push(Vec::new()); + frames.push(Frame::Blockquote { blocks: Vec::new() }); } Event::End(TagEnd::BlockQuote(_)) => { - let children = inline_stack.pop().unwrap_or_default(); - nodes.push(Node::Blockquote { children }); - in_blockquote = false; + if let Some(Frame::Blockquote { blocks }) = frames.pop() { + push_block(&mut nodes, &mut frames, Node::Blockquote { blocks }); + } } Event::Rule => { - nodes.push(Node::HorizontalRule); + push_block(&mut nodes, &mut frames, Node::HorizontalRule); } // Table elements @@ -168,11 +251,15 @@ impl MarkdownDocument { let headers = std::mem::take(&mut table_headers); let rows = std::mem::take(&mut table_rows); let col_widths = Self::table_col_widths(&headers, &rows); - nodes.push(Node::Table { - headers, - rows, - col_widths, - }); + push_block( + &mut nodes, + &mut frames, + Node::Table { + headers, + rows, + col_widths, + }, + ); in_table = false; } Event::Start(Tag::TableHead) => { @@ -281,12 +368,15 @@ impl MarkdownDocument { /// Convert a node to flat text (in characters, not bytes). pub(crate) fn node_to_flat_text(node: &Node, text: &mut String) { match node { - Node::Heading { children, .. } - | Node::Paragraph { children } - | Node::Blockquote { children } => { + Node::Heading { children, .. } | Node::Paragraph { children } => { Self::inlines_to_flat_text(children, text); text.push('\n'); } + Node::Blockquote { blocks } => { + for block in blocks { + Self::node_to_flat_text(block, text); + } + } Node::CodeBlock { code, .. } => { for line in code.lines() { text.push_str(line); @@ -295,8 +385,9 @@ impl MarkdownDocument { } Node::List { items, .. } => { for item in items { - Self::inlines_to_flat_text(item, text); - text.push('\n'); + for block in &item.blocks { + Self::node_to_flat_text(block, text); + } } } Node::Table { headers, rows, .. } => { @@ -473,9 +564,176 @@ let x = 1; assert_eq!(doc.node_offsets.first().copied(), Some(0)); } - use super::super::types::{FmValue, Frontmatter, Node}; + use super::super::types::{FmValue, Frontmatter, ListItem, Node}; use super::split_frontmatter; + fn expect_list(node: &Node) -> (bool, u64, &[ListItem]) { + match node { + Node::List { + ordered, + start, + items, + } => (*ordered, *start, items), + _ => panic!("expected a list"), + } + } + + fn item_text(item: &ListItem) -> String { + let mut out = String::new(); + for block in &item.blocks { + MarkdownDocument::node_to_flat_text(block, &mut out); + } + out + } + + /// A nested list used to clobber the list around it: the inner `Start(List)` + /// reset the single flat list state, so the outer list lost its items, took + /// the inner list's bullet marker, and closed early. Every item after the + /// nesting point fell out of the list and rendered as a bare paragraph. + #[test] + fn nested_list_keeps_the_list_around_it_intact() { + let content = "\ +1. First question. + +2. Second, with sub-points: + - changed since + - paging + +3. Third question. + +4. Fourth question. +"; + let doc = MarkdownDocument::parse(content); + + // The whole thing is one top-level list: nothing leaked out of it. + assert_eq!(doc.nodes.len(), 1, "expected a single top-level list"); + let (ordered, start, items) = expect_list(&doc.nodes[0]); + assert!(ordered, "the outer list is numbered"); + assert_eq!(start, 1); + assert_eq!(items.len(), 4); + + // Item 2 holds its own text plus the nested list, in that order. + assert_eq!(items[1].blocks.len(), 2); + assert!(matches!(items[1].blocks[0], Node::Paragraph { .. })); + let (inner_ordered, _, inner_items) = expect_list(&items[1].blocks[1]); + assert!(!inner_ordered, "the nested list is a bullet list"); + assert_eq!(inner_items.len(), 2); + + // The items after the nesting point are still items, with their text. + assert_eq!(item_text(&items[2]), "Third question.\n"); + assert_eq!(item_text(&items[3]), "Fourth question.\n"); + } + + /// Tight items (no blank line between them) carry their text as bare inline + /// events; each still ends up as one paragraph block inside its item. + #[test] + fn tight_and_multi_paragraph_items_become_blocks() { + let doc = MarkdownDocument::parse("- one\n- two\n - nested\n"); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert_eq!(items.len(), 2); + assert_eq!(item_text(&items[0]), "one\n"); + assert_eq!(items[1].blocks.len(), 2, "text plus the nested list"); + + let doc = MarkdownDocument::parse("- first para\n\n second para\n"); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert_eq!(items[0].blocks.len(), 2); + assert_eq!(item_text(&items[0]), "first para\nsecond para\n"); + } + + /// A fenced block indented under an item belongs to that item. It used to be + /// hoisted to the document root and drawn after the list it sat inside. + #[test] + fn code_block_stays_inside_its_item() { + let doc = MarkdownDocument::parse("1. Run it:\n\n ```sh\n cargo test\n ```\n"); + assert_eq!(doc.nodes.len(), 1); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert!(matches!( + items[0].blocks.as_slice(), + [Node::Paragraph { .. }, Node::CodeBlock { .. }] + )); + } + + /// A quote holds blocks, so its paragraphs stay separate instead of being + /// merged into one inline run, and a quoted list stays inside the quote + /// rather than being emitted after it. + #[test] + fn blockquote_keeps_its_blocks() { + let doc = MarkdownDocument::parse("> first para\n>\n> second para\n"); + assert_eq!(doc.nodes.len(), 1); + let Node::Blockquote { blocks } = &doc.nodes[0] else { + panic!("expected a blockquote"); + }; + assert_eq!(blocks.len(), 2); + assert_eq!(doc.plain_text, "first para\nsecond para\n"); + + let doc = MarkdownDocument::parse("> Note:\n>\n> - one\n> - two\n"); + assert_eq!(doc.nodes.len(), 1, "the list must not escape the quote"); + let Node::Blockquote { blocks } = &doc.nodes[0] else { + panic!("expected a blockquote"); + }; + assert!(matches!( + blocks.as_slice(), + [Node::Paragraph { .. }, Node::List { .. }] + )); + } + + /// The containers nest both ways round. + #[test] + fn quotes_and_lists_nest_in_each_other() { + let doc = MarkdownDocument::parse("1. Step:\n\n > watch out\n\n2. Next\n"); + assert_eq!(doc.nodes.len(), 1); + let (_, _, items) = expect_list(&doc.nodes[0]); + assert_eq!(items.len(), 2); + assert!(matches!( + items[0].blocks.as_slice(), + [Node::Paragraph { .. }, Node::Blockquote { .. }] + )); + assert_eq!(item_text(&items[0]), "Step:\nwatch out\n"); + } + + /// Markers follow the source numbering rather than always restarting at 1. + #[test] + fn ordered_list_keeps_its_first_number() { + let doc = MarkdownDocument::parse("3. three\n4. four\n"); + let (ordered, start, items) = expect_list(&doc.nodes[0]); + assert!(ordered); + assert_eq!(start, 3); + assert_eq!(items.len(), 2); + } + + /// Selection maps a character offset onto `plain_text`, so every node's + /// reported length must add up to it, nested blocks included. + #[test] + fn nested_block_lengths_match_the_flat_text() { + let content = "\ +# Title + +1. First + +2. Second: + - a + - b + + ```sh + run me + ``` + +3. Third + +> A quote, +> +> in two paragraphs. +"; + let doc = MarkdownDocument::parse(content); + let total: usize = doc + .nodes + .iter() + .map(MarkdownDocument::node_text_length) + .sum(); + assert_eq!(total, doc.plain_text.chars().count()); + assert!(doc.plain_text.contains("run me")); + } + #[test] fn detects_frontmatter_and_keeps_markdown() { let content = "\ diff --git a/crates/okena-markdown/src/render.rs b/crates/okena-markdown/src/render.rs index 6a7349d3d..b51e142bb 100644 --- a/crates/okena-markdown/src/render.rs +++ b/crates/okena-markdown/src/render.rs @@ -6,13 +6,14 @@ use gpui_component::{h_flex, v_flex}; use okena_core::theme::ThemeColors; use okena_highlight::styled::build_styled_text_with_backgrounds; use okena_highlight::syntax::HighlightedSpan; +use okena_ui::code_block::code_block_container; use okena_ui::tokens::ui_text_md; use super::style::{ - MdColors, body_line_height, body_size, heading_style, inline_code_size, node_spacing, - table_line_height, + MdColors, body_line_height, body_size, code_block_size, heading_style, inline_code_size, + node_spacing, table_line_height, }; -use super::types::{FmValue, Frontmatter, Inline, Node, char_len}; +use super::types::{FmValue, Frontmatter, Inline, ListItem, Node, char_len}; use super::{MarkdownDocument, MarkdownTextRun, RenderedNode, RenderedTextUnit}; /// Height of one code line. Code blocks are laid out line by line (each line is @@ -130,6 +131,38 @@ fn word_tokens(text: &str) -> Vec<&str> { tokens } +/// Text style a block inherits from the container it sits in. +/// +/// Quoted content reads dimmer and italic, and that has to travel down to the +/// blocks inside the quote rather than being painted over them: a paragraph sets +/// its own text colour, so a colour on an ancestor would lose to it. +#[derive(Clone, Copy)] +struct BlockStyle { + text_color: u32, + italic: bool, +} + +impl BlockStyle { + fn body(t: &ThemeColors) -> Self { + Self { + text_color: MdColors::new(t).body, + italic: false, + } + } + + fn quoted(t: &ThemeColors) -> Self { + Self { + text_color: MdColors::new(t).muted, + italic: true, + } + } + + fn apply(self, el: Div) -> Div { + el.text_color(rgb(self.text_color)) + .when(self.italic, |el| el.italic()) + } +} + /// Narrow a character selection range to the `len` characters at `offset`. fn sub_selection( selection: Option<(usize, usize)>, @@ -186,217 +219,20 @@ impl MarkdownDocument { language, code, highlighted, - } => { - // Return code blocks with individual lines for per-line selection - let selection_bg = rgba(0x3390ff40); - let mut lines = Vec::new(); - let mut line_offset = offset; - - for (line_idx, line) in code.lines().enumerate() { - let line_len = char_len(line); - let line_end = line_offset + line_len + 1; // +1 for newline - - let line_sel = node_selection.and_then(|(s, e)| { - let rel_offset = line_offset - offset; - let rel_end = rel_offset + line_len + 1; - if e <= rel_offset || s >= rel_end { - None - } else { - Some((s.saturating_sub(rel_offset), (e - rel_offset).min(line_len))) - } - }); - - let spans = highlighted.get(line_idx).map(Vec::as_slice).unwrap_or(&[]); - let display_line = if line.is_empty() { " " } else { line }; - let styled = if !spans.is_empty() { - highlighted_code_line(line, spans, line_sel, selection_bg) - } else { - plain_text_run(display_line, line_sel, selection_bg) - }; - let text_runs = vec![MarkdownTextRun::new( - styled.layout().clone(), - line.to_string(), - line_offset, - )]; - let line_div = div().h(CODE_LINE_HEIGHT).child(styled); - - lines.push(RenderedTextUnit { - div: line_div, - start_offset: line_offset, - end_offset: line_end, - text_runs, - }); - line_offset = line_end; - } - - RenderedNode::CodeBlock { - language: language.clone(), - lines, - } - } + } => RenderedNode::CodeBlock { + language: language.clone(), + // Individual lines, for per-line selection. + lines: Self::code_line_units(code, highlighted, node_selection, offset), + }, Node::Table { headers, rows, col_widths, } => { - // Return tables with individual rows for per-row selection. - // Column widths are precomputed at parse time. - let c = MdColors::new(t); - let mut row_offset = offset; - let mut rendered_rows = Vec::new(); - let mut rendered_header = None; - - // Header row - if !headers.is_empty() { - let header_len: usize = headers - .iter() - .map(|h| Self::inlines_text_length(h)) - .sum::() - + headers.len().saturating_sub(1) - + 1; // tabs + newline - let header_end = row_offset + header_len; - - let header_sel = node_selection.and_then(|(s, e)| { - let rel_start = row_offset - offset; - let rel_end = rel_start + header_len; - if e <= rel_start || s >= rel_end { - None - } else { - Some((s.saturating_sub(rel_start), (e - rel_start).min(header_len))) - } - }); - - let mut header_row = h_flex(); - let mut header_runs = Vec::new(); - let mut cell_offset = 0usize; - for (i, header) in headers.iter().enumerate() { - let cell_len = - Self::inlines_text_length(header) + if i > 0 { 1 } else { 0 }; - let cell_sel = header_sel.and_then(|(s, e)| { - let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; - let cell_end = cell_offset + cell_len; - if e <= cell_start || s >= cell_end { - None - } else { - Some(( - s.saturating_sub(cell_start), - (e - cell_start).min(Self::inlines_text_length(header)), - )) - } - }); - - let width = col_widths.get(i).copied().unwrap_or(10); - let min_w = ((width * 8) + 24).max(80) as f32; - header_row = header_row.child( - div().min_w(px(min_w)).px(px(12.0)).py(px(8.0)).child( - Self::render_inlines_with_selection_and_targets( - header, - t, - cx, - cell_sel, - row_offset + cell_offset + if i > 0 { 1 } else { 0 }, - &mut header_runs, - ) - .text_size(ui_text_md(cx)) - .line_height(table_line_height(cx)) - .font_weight(FontWeight::SEMIBOLD) - .text_color(rgb(c.heading)), - ), - ); - cell_offset += cell_len; - } - - let header_div = header_row - .bg(rgb(c.surface)) - .border_b_1() - .border_color(rgb(c.surface_border)); - rendered_header = Some(RenderedTextUnit { - div: header_div, - start_offset: row_offset, - end_offset: header_end, - text_runs: header_runs, - }); - row_offset = header_end; - } - - // Data rows - for (row_idx, row) in rows.iter().enumerate() { - let row_len: usize = row - .iter() - .map(|cell| Self::inlines_text_length(cell)) - .sum::() - + row.len().saturating_sub(1) - + 1; // tabs + newline - let row_end = row_offset + row_len; - - let row_sel = node_selection.and_then(|(s, e)| { - let rel_start = row_offset - offset; - let rel_end = rel_start + row_len; - if e <= rel_start || s >= rel_end { - None - } else { - Some((s.saturating_sub(rel_start), (e - rel_start).min(row_len))) - } - }); - - let mut row_div = h_flex(); - let mut row_runs = Vec::new(); - if row_idx % 2 == 1 { - row_div = row_div.bg(rgb(c.surface)); - } - if row_idx < rows.len() - 1 { - row_div = row_div.border_b_1().border_color(rgb(c.surface_border)); - } - - let mut cell_offset = 0usize; - for (i, cell) in row.iter().enumerate() { - let cell_len = Self::inlines_text_length(cell) + if i > 0 { 1 } else { 0 }; - let cell_sel = row_sel.and_then(|(s, e)| { - let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; - let cell_end = cell_offset + cell_len; - if e <= cell_start || s >= cell_end { - None - } else { - Some(( - s.saturating_sub(cell_start), - (e - cell_start).min(Self::inlines_text_length(cell)), - )) - } - }); - - let width = col_widths.get(i).copied().unwrap_or(10); - let min_w = ((width * 8) + 24).max(80) as f32; - row_div = row_div.child( - div().min_w(px(min_w)).px(px(12.0)).py(px(6.0)).child( - Self::render_inlines_with_selection_and_targets( - cell, - t, - cx, - cell_sel, - row_offset + cell_offset + if i > 0 { 1 } else { 0 }, - &mut row_runs, - ) - .text_size(ui_text_md(cx)) - .line_height(table_line_height(cx)) - .text_color(rgb(c.body)), - ), - ); - cell_offset += cell_len; - } - - rendered_rows.push(RenderedTextUnit { - div: row_div, - start_offset: row_offset, - end_offset: row_end, - text_runs: row_runs, - }); - row_offset = row_end; - } - - RenderedNode::Table { - header: rendered_header, - rows: rendered_rows, - } + // Individual rows, for per-row selection. + let (header, rows) = + Self::table_units(headers, rows, col_widths, t, cx, node_selection, offset); + RenderedNode::Table { header, rows } } _ => { // Other nodes are simple blocks @@ -408,6 +244,7 @@ impl MarkdownDocument { node_selection, offset, &mut text_runs, + BlockStyle::body(t), ); RenderedNode::Simple { div: node_div, @@ -421,14 +258,238 @@ impl MarkdownDocument { Some(rendered) } + /// Lay out a code block line by line, each line its own selectable unit. + /// + /// `selection` is a character range relative to the start of the block; + /// `base_offset` is the block's own offset in the document's flat text. + fn code_line_units( + code: &str, + highlighted: &[Vec], + selection: Option<(usize, usize)>, + base_offset: usize, + ) -> Vec { + let selection_bg = rgba(0x3390ff40); + let mut lines = Vec::new(); + let mut line_offset = base_offset; + + for (line_idx, line) in code.lines().enumerate() { + let line_len = char_len(line); + let line_end = line_offset + line_len + 1; // +1 for newline + + let line_sel = selection.and_then(|(s, e)| { + let rel_offset = line_offset - base_offset; + let rel_end = rel_offset + line_len + 1; + if e <= rel_offset || s >= rel_end { + None + } else { + Some((s.saturating_sub(rel_offset), (e - rel_offset).min(line_len))) + } + }); + + let spans = highlighted.get(line_idx).map(Vec::as_slice).unwrap_or(&[]); + let display_line = if line.is_empty() { " " } else { line }; + let styled = if !spans.is_empty() { + highlighted_code_line(line, spans, line_sel, selection_bg) + } else { + plain_text_run(display_line, line_sel, selection_bg) + }; + let text_runs = vec![MarkdownTextRun::new( + styled.layout().clone(), + line.to_string(), + line_offset, + )]; + let line_div = div().h(CODE_LINE_HEIGHT).child(styled); + + lines.push(RenderedTextUnit { + div: line_div, + start_offset: line_offset, + end_offset: line_end, + text_runs, + }); + line_offset = line_end; + } + + lines + } + + /// Lay out a table row by row, each row its own selectable unit. Column + /// widths come precomputed from parse time. + /// + /// `selection` is relative to the start of the table; `base_offset` is the + /// table's offset in the document's flat text. + #[allow(clippy::too_many_arguments)] + fn table_units( + headers: &[Vec], + rows: &[Vec>], + col_widths: &[usize], + t: &ThemeColors, + cx: &App, + selection: Option<(usize, usize)>, + base_offset: usize, + ) -> (Option, Vec) { + let c = MdColors::new(t); + let mut row_offset = base_offset; + let mut rendered_rows = Vec::new(); + let mut rendered_header = None; + + // Header row + if !headers.is_empty() { + let header_len: usize = headers + .iter() + .map(|h| Self::inlines_text_length(h)) + .sum::() + + headers.len().saturating_sub(1) + + 1; // tabs + newline + let header_end = row_offset + header_len; + + let header_sel = selection.and_then(|(s, e)| { + let rel_start = row_offset - base_offset; + let rel_end = rel_start + header_len; + if e <= rel_start || s >= rel_end { + None + } else { + Some((s.saturating_sub(rel_start), (e - rel_start).min(header_len))) + } + }); + + let mut header_row = h_flex(); + let mut header_runs = Vec::new(); + let mut cell_offset = 0usize; + for (i, header) in headers.iter().enumerate() { + let cell_len = Self::inlines_text_length(header) + if i > 0 { 1 } else { 0 }; + let cell_sel = header_sel.and_then(|(s, e)| { + let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; + let cell_end = cell_offset + cell_len; + if e <= cell_start || s >= cell_end { + None + } else { + Some(( + s.saturating_sub(cell_start), + (e - cell_start).min(Self::inlines_text_length(header)), + )) + } + }); + + let width = col_widths.get(i).copied().unwrap_or(10); + let min_w = ((width * 8) + 24).max(80) as f32; + header_row = header_row.child( + div().min_w(px(min_w)).px(px(12.0)).py(px(8.0)).child( + Self::render_inlines_with_selection_and_targets( + header, + t, + cx, + cell_sel, + row_offset + cell_offset + if i > 0 { 1 } else { 0 }, + &mut header_runs, + BlockStyle::body(t), + ) + .text_size(ui_text_md(cx)) + .line_height(table_line_height(cx)) + .font_weight(FontWeight::SEMIBOLD) + .text_color(rgb(c.heading)), + ), + ); + cell_offset += cell_len; + } + + let header_div = header_row + .bg(rgb(c.surface)) + .border_b_1() + .border_color(rgb(c.surface_border)); + rendered_header = Some(RenderedTextUnit { + div: header_div, + start_offset: row_offset, + end_offset: header_end, + text_runs: header_runs, + }); + row_offset = header_end; + } + + // Data rows + for (row_idx, row) in rows.iter().enumerate() { + let row_len: usize = row + .iter() + .map(|cell| Self::inlines_text_length(cell)) + .sum::() + + row.len().saturating_sub(1) + + 1; // tabs + newline + let row_end = row_offset + row_len; + + let row_sel = selection.and_then(|(s, e)| { + let rel_start = row_offset - base_offset; + let rel_end = rel_start + row_len; + if e <= rel_start || s >= rel_end { + None + } else { + Some((s.saturating_sub(rel_start), (e - rel_start).min(row_len))) + } + }); + + let mut row_div = h_flex(); + let mut row_runs = Vec::new(); + if row_idx % 2 == 1 { + row_div = row_div.bg(rgb(c.surface)); + } + if row_idx < rows.len() - 1 { + row_div = row_div.border_b_1().border_color(rgb(c.surface_border)); + } + + let mut cell_offset = 0usize; + for (i, cell) in row.iter().enumerate() { + let cell_len = Self::inlines_text_length(cell) + if i > 0 { 1 } else { 0 }; + let cell_sel = row_sel.and_then(|(s, e)| { + let cell_start = cell_offset + if i > 0 { 1 } else { 0 }; + let cell_end = cell_offset + cell_len; + if e <= cell_start || s >= cell_end { + None + } else { + Some(( + s.saturating_sub(cell_start), + (e - cell_start).min(Self::inlines_text_length(cell)), + )) + } + }); + + let width = col_widths.get(i).copied().unwrap_or(10); + let min_w = ((width * 8) + 24).max(80) as f32; + row_div = row_div.child( + div().min_w(px(min_w)).px(px(12.0)).py(px(6.0)).child( + Self::render_inlines_with_selection_and_targets( + cell, + t, + cx, + cell_sel, + row_offset + cell_offset + if i > 0 { 1 } else { 0 }, + &mut row_runs, + BlockStyle::body(t), + ) + .text_size(ui_text_md(cx)) + .line_height(table_line_height(cx)) + .text_color(rgb(c.body)), + ), + ); + cell_offset += cell_len; + } + + rendered_rows.push(RenderedTextUnit { + div: row_div, + start_offset: row_offset, + end_offset: row_end, + text_runs: row_runs, + }); + row_offset = row_end; + } + + (rendered_header, rendered_rows) + } + /// Calculate the text length of a node (for selection offset tracking, in characters). pub(crate) fn node_text_length(node: &Node) -> usize { match node { - Node::Heading { level: _, children } - | Node::Paragraph { children } - | Node::Blockquote { children } => { + Node::Heading { level: _, children } | Node::Paragraph { children } => { Self::inlines_text_length(children) + 1 // +1 for newline } + Node::Blockquote { blocks } => blocks.iter().map(Self::node_text_length).sum(), Node::CodeBlock { code, .. } => { // Sum of character lengths of each line + 1 newline per line code.lines() @@ -436,10 +497,7 @@ impl MarkdownDocument { .sum::() .max(1) } - Node::List { items, .. } => items - .iter() - .map(|item| Self::inlines_text_length(item) + 1) - .sum(), + Node::List { items, .. } => items.iter().map(Self::list_item_text_length).sum(), Node::Table { headers, rows, .. } => { let header_len: usize = headers.iter().map(|h| Self::inlines_text_length(h)).sum::() + headers.len().saturating_sub(1) // tabs @@ -459,6 +517,12 @@ impl MarkdownDocument { } } + /// Text length of one list item: the sum of its blocks, each of which + /// already accounts for its own trailing newline. + pub(crate) fn list_item_text_length(item: &ListItem) -> usize { + item.blocks.iter().map(Self::node_text_length).sum() + } + /// Calculate the text length of inline elements (in characters, not bytes). pub(crate) fn inlines_text_length(inlines: &[Inline]) -> usize { inlines @@ -474,7 +538,10 @@ impl MarkdownDocument { .sum() } - /// Render a node with selection highlighting. + /// Render a node with selection highlighting. `style` is what the container + /// around the node imposes on its text, which is how a quote dims and + /// italicises the blocks inside it. + #[allow(clippy::too_many_arguments)] fn render_node_with_selection( node: &Node, t: &ThemeColors, @@ -482,6 +549,7 @@ impl MarkdownDocument { selection: Option<(usize, usize)>, base_offset: usize, text_runs: &mut Vec, + style: BlockStyle, ) -> Div { let c = MdColors::new(t); match node { @@ -509,9 +577,14 @@ impl MarkdownDocument { selection, base_offset, text_runs, + style, ) .w_full(), - Node::List { ordered, items } => { + Node::List { + ordered, + start, + items, + } => { // One marker column for both list kinds, right-aligned in it, so // the text hangs at the same indent whatever the marker is — // including two-digit numbers. @@ -519,24 +592,36 @@ impl MarkdownDocument { let mut list = v_flex().w_full().gap(px(6.0)).pl(px(4.0)); let mut offset = 0usize; - for (i, item_inlines) in items.iter().enumerate() { - let item_len = Self::inlines_text_length(item_inlines) + 1; - let item_sel = selection.and_then(|(s, e)| { - if e <= offset || s >= offset + item_len { - None - } else { - Some(( - s.saturating_sub(offset), - (e - offset).min(item_len - 1), // -1 to exclude newline - )) - } - }); + for (i, item) in items.iter().enumerate() { + let item_len = Self::list_item_text_length(item); + let item_sel = sub_selection(selection, offset, item_len); let marker = if *ordered { - format!("{}.", i + 1) + // Honour the source numbering: a list written `3.` first + // keeps starting at 3. + format!("{}.", start.saturating_add(i as u64)) } else { "\u{2022}".to_string() }; + + // An item is a block container: several paragraphs, a code + // block, or a nested list all stack in this column. + let mut content = v_flex().flex_1().min_w_0().gap(px(6.0)); + let mut block_offset = 0usize; + for block in &item.blocks { + let block_len = Self::node_text_length(block); + content = content.child(Self::render_node_with_selection( + block, + t, + cx, + sub_selection(item_sel, block_offset, block_len), + base_offset + offset + block_offset, + text_runs, + style, + )); + block_offset += block_len; + } + list = list.child( div() .flex() @@ -554,47 +639,91 @@ impl MarkdownDocument { .text_right() .child(marker), ) - .child( - Self::render_inlines_with_selection_and_targets( - item_inlines, - t, - cx, - item_sel, - base_offset + offset, - text_runs, - ) - .flex_1(), - ), + .child(content), ); offset += item_len; } list } - Node::Blockquote { children } => div() - .pl(px(14.0)) - .border_l_2() - .border_color(rgb(c.surface_border)) - .child( - Self::render_inlines_with_selection_and_targets( - children, + Node::Blockquote { blocks } => { + // A quote stacks whatever it holds, so several quoted paragraphs + // stay separate and a quoted list keeps its markers. + let quoted = BlockStyle::quoted(t); + let mut quote = v_flex() + .w_full() + .gap(px(8.0)) + .pl(px(14.0)) + .border_l_2() + .border_color(rgb(c.surface_border)); + let mut block_offset = 0usize; + for block in blocks { + let block_len = Self::node_text_length(block); + quote = quote.child(Self::render_node_with_selection( + block, t, cx, - selection, - base_offset, + sub_selection(selection, block_offset, block_len), + base_offset + block_offset, text_runs, - ) - .w_full() - .text_color(rgb(c.muted)) - .italic(), - ), + quoted, + )); + block_offset += block_len; + } + quote + } // Whitespace is what separates sections here, so an explicit rule // stays as a hairline that barely registers. Node::HorizontalRule => div().w_full().h(px(1.0)).bg(rgb(c.rule)), Node::Frontmatter { block, .. } => { Self::render_frontmatter(block, t, cx, selection, base_offset, text_runs) } - // These are rendered by the specialized branches in `render_node`. - Node::CodeBlock { .. } | Node::Table { .. } => div(), + // A top-level code block or table is drawn by `render_node`, which + // hands the viewer its lines/rows as separate selectable units. One + // nested in a list item has no such unit of its own, so it is drawn + // here (chrome included), and folds its runs into the parent's. + Node::CodeBlock { + language, + code, + highlighted, + } => { + let mut lines = Vec::new(); + for unit in Self::code_line_units(code, highlighted, selection, base_offset) { + text_runs.extend(unit.text_runs); + lines.push(unit.div); + } + code_block_container(language.as_deref(), t, cx) + .w_full() + .child( + v_flex() + .px(px(12.0)) + .py(px(8.0)) + .font_family("monospace") + .text_size(code_block_size(cx)) + .text_color(rgb(c.body)) + .children(lines), + ) + } + Node::Table { + headers, + rows, + col_widths, + } => { + let (header, rows) = + Self::table_units(headers, rows, col_widths, t, cx, selection, base_offset); + let mut units = Vec::new(); + for unit in header.into_iter().chain(rows) { + text_runs.extend(unit.text_runs); + units.push(unit.div); + } + v_flex() + .items_start() + .max_w_full() + .overflow_hidden() + .rounded(px(6.0)) + .border_1() + .border_color(rgb(c.surface_border)) + .children(units) + } } } @@ -893,6 +1022,7 @@ impl MarkdownDocument { list } + #[allow(clippy::too_many_arguments)] fn render_inlines_with_selection_and_targets( inlines: &[Inline], t: &ThemeColors, @@ -900,6 +1030,7 @@ impl MarkdownDocument { selection: Option<(usize, usize)>, base_offset: usize, text_runs: &mut Vec, + style: BlockStyle, ) -> Div { let mut elements: Vec
= Vec::new(); Self::push_inlines( @@ -913,7 +1044,7 @@ impl MarkdownDocument { text_runs, ); - div() + let row = div() .flex() .flex_wrap() // `min-width: 0` lets this inline-flow container shrink below its @@ -925,8 +1056,8 @@ impl MarkdownDocument { .items_baseline() .text_size(body_size(cx)) .line_height(body_line_height(cx)) - .text_color(rgb(MdColors::new(t).body)) - .children(elements) + .children(elements); + style.apply(row) } /// Text length of one inline element, in characters. diff --git a/crates/okena-markdown/src/style.rs b/crates/okena-markdown/src/style.rs index 61b3646cf..4213ebc61 100644 --- a/crates/okena-markdown/src/style.rs +++ b/crates/okena-markdown/src/style.rs @@ -35,6 +35,12 @@ pub(crate) fn inline_code_size(cx: &App) -> Pixels { ui_text(INLINE_CODE_PT, cx) } +/// Code size for blocks this crate draws itself, one nested inside a list +/// item, say. A top-level block is sized by the viewer's own file font size. +pub(crate) fn code_block_size(cx: &App) -> Pixels { + ui_text(13.0, cx) +} + /// Table cells run at UI size, with their own leading — body leading would make /// a dense table too airy. pub(crate) fn table_line_height(cx: &App) -> Pixels { diff --git a/crates/okena-markdown/src/types.rs b/crates/okena-markdown/src/types.rs index 79d836fd6..7f8f072f9 100644 --- a/crates/okena-markdown/src/types.rs +++ b/crates/okena-markdown/src/types.rs @@ -24,7 +24,10 @@ pub(crate) enum Node { }, List { ordered: bool, - items: Vec>, + /// First number of an ordered list: `3.` starts the markers at 3. + /// Always 1 for a bullet list. + start: u64, + items: Vec, }, Table { headers: Vec>, @@ -33,8 +36,11 @@ pub(crate) enum Node { /// rendering does not re-measure every cell on every frame. col_widths: Vec, }, + /// A quote is a block container too: it can hold several paragraphs, or a + /// list. Collecting only its inlines merged every quoted paragraph onto one + /// line and left a quoted list to render after the quote instead of in it. Blockquote { - children: Vec, + blocks: Vec, }, HorizontalRule, /// YAML frontmatter at the top of the document, rendered as a metadata card @@ -48,6 +54,16 @@ pub(crate) enum Node { }, } +/// One item of a list, as a sequence of blocks rather than a single inline run. +/// +/// An item is a block container in markdown: it can hold several paragraphs, a +/// code block, or (the case that matters most) a nested list. Flattening it to +/// inlines is what used to make a nested list overwrite the list containing it. +#[derive(Clone)] +pub(crate) struct ListItem { + pub(crate) blocks: Vec, +} + /// Parsed YAML frontmatter block. #[derive(Clone)] pub(crate) enum Frontmatter { diff --git a/crates/okena-markdown/tests/inline_layout.rs b/crates/okena-markdown/tests/inline_layout.rs index 5a6248ae5..190db0290 100644 --- a/crates/okena-markdown/tests/inline_layout.rs +++ b/crates/okena-markdown/tests/inline_layout.rs @@ -222,6 +222,82 @@ fn table_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { assert_eq!(doc.plain_text, "Hé\tB🙂\none\ttwo\n"); } +/// A nested list renders inside the item that holds it, and the text runs of +/// every block (nested ones included) still carry their global character +/// offsets, which is what selection and copy are built on. +#[gpui::test] +fn nested_list_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { + let doc = MarkdownDocument::parse("1. First\n\n2. Second:\n - a\n - b\n\n3. Third\n"); + assert_eq!(doc.plain_text, "First\nSecond:\na\nb\nThird\n"); + // One list, not a list plus the paragraphs that used to fall out of it. + assert_eq!(doc.node_count(), 1); + + let captured: Rc>> = Default::default(); + let captured_for_draw = captured.clone(); + let vcx = cx.add_empty_window(); + + vcx.draw( + Point::default(), + Size { + width: AvailableSpace::Definite(px(500.0)), + height: AvailableSpace::MinContent, + }, + |_window, cx| { + let Some(RenderedNode::Simple { div, text_runs, .. }) = + doc.render_node(0, &DARK_THEME, cx, None) + else { + return div(); + }; + captured_for_draw.borrow_mut().extend(text_runs); + div + }, + ); + + let starts = captured + .borrow() + .iter() + .map(run_start_offset) + .collect::>(); + assert_eq!(starts, [0, 6, 14, 16, 18]); +} + +/// A quote renders the blocks it holds, so a quoted list keeps its markers and +/// the offsets behind selection stay in step with the flat text. +#[gpui::test] +fn blockquote_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { + let doc = MarkdownDocument::parse("> Note:\n>\n> - one\n> - two\n"); + assert_eq!(doc.plain_text, "Note:\none\ntwo\n"); + assert_eq!(doc.node_count(), 1); + + let captured: Rc>> = Default::default(); + let captured_for_draw = captured.clone(); + let vcx = cx.add_empty_window(); + + vcx.draw( + Point::default(), + Size { + width: AvailableSpace::Definite(px(500.0)), + height: AvailableSpace::MinContent, + }, + |_window, cx| { + let Some(RenderedNode::Simple { div, text_runs, .. }) = + doc.render_node(0, &DARK_THEME, cx, None) + else { + return div(); + }; + captured_for_draw.borrow_mut().extend(text_runs); + div + }, + ); + + let starts = captured + .borrow() + .iter() + .map(run_start_offset) + .collect::>(); + assert_eq!(starts, [0, 6, 10]); +} + #[gpui::test] fn frontmatter_text_runs_follow_flat_text_offsets(cx: &mut TestAppContext) { let doc = MarkdownDocument::parse("---\ntitle: Žluť\nitems:\n - one\n---\n"); diff --git a/crates/okena-terminal/src/pty_manager.rs b/crates/okena-terminal/src/pty_manager.rs index c262644d9..804fef37e 100644 --- a/crates/okena-terminal/src/pty_manager.rs +++ b/crates/okena-terminal/src/pty_manager.rs @@ -2661,7 +2661,8 @@ mod tests { program: "/bin/sh".to_string(), args: vec![ "-c".to_string(), - "sleep 30 & echo $! > \"$1\"; wait".to_string(), + // Publish only the complete PID; redirection creates an empty file first. + "sleep 30 & echo $! > \"$1.tmp\"; mv \"$1.tmp\" \"$1\"; wait".to_string(), "okena-test".to_string(), child_pid_file.to_string_lossy().into_owned(), ], diff --git a/crates/okena-workspace/src/persistence.rs b/crates/okena-workspace/src/persistence.rs index 0c7ea9ae5..2a1e123ca 100644 --- a/crates/okena-workspace/src/persistence.rs +++ b/crates/okena-workspace/src/persistence.rs @@ -1793,6 +1793,15 @@ mod tests { let root = std::env::temp_dir().join(format!("okena-wt-registry-{}", uuid::Uuid::new_v4())); std::fs::create_dir_all(&root).expect("create fixture root"); + // Git's worktree registry reports resolved paths, and recovery + // matches a row's path against them without touching the + // filesystem, so that a checkout deleted from disk stays + // sweepable. On macOS the temp directory is handed out behind a + // symlink (`/var/folders/...` is `/private/var/folders/...`), so a + // fixture built from it writes rows the registry can never match + // and the recovery under test never runs. Start from the resolved + // path, which is what a checkout anywhere else already is. + let root = std::fs::canonicalize(&root).expect("resolve fixture root"); Self { root } } diff --git a/tests/cli_over_the_remote_api.rs b/tests/cli_over_the_remote_api.rs index 4199f595d..1f4864ebe 100644 --- a/tests/cli_over_the_remote_api.rs +++ b/tests/cli_over_the_remote_api.rs @@ -43,9 +43,37 @@ impl Daemon { let daemon = Self { child, root }; daemon.wait_for_remote_json(); + daemon.wait_until_the_cli_gets_an_answer(); daemon } + /// Ready means a CLI call has actually been answered. + /// + /// `remote.json` says only that the port is published; the daemon is still + /// finishing startup behind it, and the first CLI call carries the one-off + /// token registration on top of the command itself. The CLI gives a request + /// 5 seconds, which a busy machine can spend on that first round trip + /// alone, so a test that starts asserting the moment the file lands fails + /// on a daemon that is merely still waking up. Spend the wait here, where + /// it proves nothing, instead of inside an assertion where it looks like a + /// verdict. + fn wait_until_the_cli_gets_an_answer(&self) { + let deadline = Instant::now() + Duration::from_secs(60); + loop { + let output = self.cli(&["ls", "--json"]); + if output.status.success() { + return; + } + if Instant::now() >= deadline { + panic!( + "the daemon never answered `okena ls --json`: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + std::thread::sleep(Duration::from_millis(100)); + } + } + fn command(root: &Path) -> Command { let mut command = Command::new(BIN); command @@ -56,9 +84,27 @@ impl Daemon { command } + /// Where the daemon keeps the active profile, mirroring + /// `okena_core::profiles::config_root`. + /// + /// That resolves through `dirs::config_dir()`, which is `$XDG_CONFIG_HOME` + /// on Linux but `~/Library/Application Support` on macOS. Both are + /// redirected into the isolated root, so the daemon is contained either + /// way; only the path to look at differs. Watching the Linux one alone made + /// every daemon-backed test in this file fail on macOS, waiting out the + /// full timeout for a file that was published elsewhere a second in. + fn profile_dir(&self) -> PathBuf { + let config_root = if cfg!(target_os = "macos") { + self.root.join("home/Library/Application Support") + } else { + self.root.join("cfg") + }; + config_root.join("okena/profiles/default") + } + /// The daemon publishes its port here, and the CLI discovers it from here. fn wait_for_remote_json(&self) { - let published = self.root.join("cfg/okena/profiles/default/remote.json"); + let published = self.profile_dir().join("remote.json"); let deadline = Instant::now() + Duration::from_secs(60); while Instant::now() < deadline { if published.exists() { @@ -90,7 +136,7 @@ impl Daemon { /// The bearer token the first CLI call registered for this daemon. fn cli_token(&self) -> String { - let path = self.root.join("cfg/okena/profiles/default/cli.json"); + let path = self.profile_dir().join("cli.json"); let config: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).expect("cli.json")) .expect("cli.json is JSON");