Merge pull request 'fix(client/windows): "Open log folder" stops opening Documents' (#31) from worktree-client-logs-folder-msix into main

Reviewed-on: #31

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();
         }
     });

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<Option<Arc<Mutex<File>>>> = OnceLock::new();
 
-/// The log directory — Settings ▸ About's "Open log folder" opens it in Explorer.
-pub(crate) fn log_dir() -> Option<PathBuf> {
+/// 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<PathBuf> {
     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\<family>\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 <path>" 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<PathBuf> {
+    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<PathBuf> {
-    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()
+        );
+    }
+}