diff --git a/clients/windows/src/app/settings.rs b/clients/windows/src/app/settings.rs index 9e970787..2d924a44 100644 --- a/clients/windows/src/app/settings.rs +++ b/clients/windows/src/app/settings.rs @@ -1045,13 +1045,18 @@ pub(crate) fn settings_page( let ss = set_screen.clone(); button("Third-party licenses").on_click(move || ss.call(Screen::Licenses)) }; - // The client log's home (%LOCALAPPDATA%\punktfunk\logs) — the file every "check the - // client log" message means, which until this row had no way in from the UI at all. - // The folder rather than the file so the rotated `.old` generation is in reach too. - // Best-effort, like the log itself: a missing dir or a failed spawn stays silent. + // The client log's home — the file every "check the client log" message means, which until + // this row had no way in from the UI at all. The folder rather than the file so the rotated + // `.old` generation is in reach too. + // + // `real_dir` (not the literal %LOCALAPPDATA% path) because Explorer lives outside our MSIX + // container: handed a path the package redirection keeps from ever existing, it silently + // opens the user's Documents folder instead of failing, which is precisely what this button + // shipped doing. The `is_dir` guard keeps that fallback unreachable — if the resolve ever + // comes back wrong, the click does nothing rather than landing somewhere misleading. + // Best-effort otherwise, like the log itself: a failed spawn stays silent. let logs_button = button("Open log folder").on_click(|| { - if let Some(dir) = crate::logfile::log_dir() { - let _ = std::fs::create_dir_all(&dir); + if let Some(dir) = crate::logfile::real_dir().filter(|d| d.is_dir()) { let _ = std::process::Command::new("explorer.exe").arg(&dir).spawn(); } }); diff --git a/clients/windows/src/logfile.rs b/clients/windows/src/logfile.rs index 4e6efddb..0fb52aef 100644 --- a/clients/windows/src/logfile.rs +++ b/clients/windows/src/logfile.rs @@ -10,6 +10,10 @@ //! Mirrors the host's convention (`%ProgramData%\punktfunk\logs`, size-capped): a file over //! 10 MB is rotated to `.old` at the next client start, one generation kept. Everything is //! best-effort — a missing/locked directory degrades to plain stderr, never a startup failure. +//! +//! Two paths, deliberately: [`log_dir`] is what we open files through, [`real_dir`] is where +//! they actually land. Under MSIX those differ, and only the second one is fit to show a user +//! or hand to Explorer. use std::fs::{File, OpenOptions}; use std::io::{self, BufRead, Write}; @@ -21,14 +25,74 @@ const ROTATE_BYTES: u64 = 10 * 1024 * 1024; static SINK: OnceLock>>> = OnceLock::new(); -/// The log directory — Settings ▸ About's "Open log folder" opens it in Explorer. -pub(crate) fn log_dir() -> Option { +/// The log directory we WRITE through: `%LOCALAPPDATA%\punktfunk\logs`. +/// +/// Correct to open files under, but NOT necessarily where the bytes land — see [`real_dir`]. +/// Anything shown to a user or handed to another process wants that one instead. +fn log_dir() -> Option { Some(PathBuf::from(std::env::var_os("LOCALAPPDATA")?).join(r"punktfunk\logs")) } +/// The log directory as it exists ON DISK — Settings ▸ About's "Open log folder" opens this in +/// Explorer, and [`path`] names it in the startup line and the failed-spawn banner. +/// +/// The shipping client is a full-trust MSIX package, and Windows redirects a packaged app's +/// `%LOCALAPPDATA%` writes into its private `…\Packages\\LocalCache\Local\…`. We create +/// and append through that redirection without ever seeing it, so [`log_dir`] is the right path +/// to WRITE to yet names a directory that never exists on disk. Explorer runs OUTSIDE the +/// container: it resolves the literal path, finds nothing, and silently falls back to the user's +/// Documents folder — which is exactly what "Open log folder" did in every packaged install, and +/// what the two "check " messages pointed at. An unpackaged dev run creates the literal +/// directory for real, which is why this only ever showed up in the field. +/// +/// Canonicalizing the directory we just created resolves through the redirection on a packaged +/// run and changes nothing on an unpackaged one, so there is no package identity to detect. +pub(crate) fn real_dir() -> Option { + let dir = log_dir()?; + std::fs::create_dir_all(&dir).ok()?; + Some(std::fs::canonicalize(&dir).map_or(dir, strip_verbatim)) +} + +/// Undo the `\\?\` that [`std::fs::canonicalize`] always prefixes. Explorer refuses a verbatim +/// path — it would take the very same silent Documents fallback [`real_dir`] exists to avoid — +/// and it is noise in a line a user is meant to read and act on. +fn strip_verbatim(p: PathBuf) -> PathBuf { + use std::path::{Component, Prefix}; + + // Scoped so the borrow ends before the `return p` below can move it. + let head = match p.components().next() { + Some(Component::Prefix(pre)) => match pre.kind() { + // `\\?\C:\…` → `C:\…` + Prefix::VerbatimDisk(drive) => Some(PathBuf::from(format!(r"{}:\", drive as char))), + // `\\?\UNC\server\share\…` → `\\server\share\…` (a roaming profile on a share). + // Built through `OsString`, which appends verbatim — `PathBuf::push` would apply + // separator logic to the bare `\\` and mangle it. + Prefix::VerbatimUNC(server, share) => { + let mut unc = std::ffi::OsString::from(r"\\"); + unc.push(server); + unc.push(r"\"); + unc.push(share); + Some(PathBuf::from(unc)) + } + // Already a plain path — nothing to undo. + _ => None, + }, + _ => None, + }; + let Some(mut out) = head else { return p }; + // `skip(1)` drops the prefix; the `RootDir` that follows it is already in `head`. + out.extend( + p.components() + .skip(1) + .filter(|c| !matches!(c, Component::RootDir)), + ); + out +} + /// The log file's path, for the "logs land here" startup line and the failed-spawn banner. +/// Resolved like [`real_dir`] — a path a user is told to check has to be the one on disk. pub(crate) fn path() -> Option { - Some(log_dir()?.join("client.log")) + Some(real_dir()?.join("client.log")) } /// Open (rotating first) and cache the sink. Called once at startup, before the tracing @@ -97,3 +161,67 @@ pub(crate) fn forward_child_stderr(stderr: impl io::Read + Send + 'static) { } }); } + +#[cfg(test)] +mod tests { + use super::*; + + /// The shape `canonicalize` actually returns for a local profile. Explorer treats a `\\?\` + /// path as unresolvable and opens Documents instead, so the prefix has to come off. + #[test] + fn verbatim_disk_prefix_comes_off() { + let p = PathBuf::from(r"\\?\C:\Users\ada\AppData\Local\punktfunk\logs"); + assert_eq!( + strip_verbatim(p), + PathBuf::from(r"C:\Users\ada\AppData\Local\punktfunk\logs") + ); + } + + /// The MSIX-redirected form is what the fix is for: same treatment, longer path. + #[test] + fn verbatim_disk_prefix_comes_off_for_the_package_local_cache() { + let p = PathBuf::from( + r"\\?\C:\Users\ada\AppData\Local\Packages\unom.Punktfunk_8wekyb3d8bbwe\LocalCache\Local\punktfunk\logs", + ); + assert_eq!( + strip_verbatim(p), + PathBuf::from( + r"C:\Users\ada\AppData\Local\Packages\unom.Punktfunk_8wekyb3d8bbwe\LocalCache\Local\punktfunk\logs" + ) + ); + } + + /// A roaming profile on a share canonicalizes to `\\?\UNC\…`; the plain UNC form is what + /// Explorer takes. `\\server\share` must survive intact — dropping either half, or letting + /// `PathBuf::push`'s separator logic at the bare `\\`, yields a path that opens nothing. + #[test] + fn verbatim_unc_prefix_becomes_a_plain_unc_path() { + let p = PathBuf::from(r"\\?\UNC\fileserv\profiles\ada\AppData\Local\punktfunk\logs"); + assert_eq!( + strip_verbatim(p), + PathBuf::from(r"\\fileserv\profiles\ada\AppData\Local\punktfunk\logs") + ); + } + + /// An unpackaged dev run resolves to a path that was never verbatim — leave it alone. + #[test] + fn plain_path_is_untouched() { + let p = PathBuf::from(r"C:\Users\ada\AppData\Local\punktfunk\logs"); + assert_eq!(strip_verbatim(p.clone()), p); + } + + /// Whatever the run, the resolved directory is one Explorer can open: it exists, and it + /// carries no verbatim prefix. This is the button's actual precondition. + #[test] + fn real_dir_is_an_openable_directory() { + let Some(dir) = real_dir() else { + return; // no LOCALAPPDATA (not a normal user session) — nothing to assert + }; + assert!(dir.is_dir(), "{} is not a directory", dir.display()); + assert!( + !dir.to_string_lossy().starts_with(r"\\?\"), + "{} kept its verbatim prefix", + dir.display() + ); + } +}