Skip to content

Commit 3640966

Browse files
maphewclaude
andauthored
PR 1: Correct open and watch behavior (v0.6) (#19)
* fix: generate standards-correct file URLs for <base href> (mdo-bs3.1.2) Replace the hand-rolled path-to-URL string transform with url::Url::from_directory_path, which percent-encodes reserved and non-ASCII characters (spaces, #, %, ?, Unicode) and handles Windows drive letters, UNC paths, and \\?\ extended-length prefixes. Paths that cannot form a valid file:// URL now omit the <base href> tag instead of emitting a broken one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: honest --open failures and rename-resilient watch mode (mdo-bs3.1.1, mdo-bs3.1.3) --open now exits non-zero when the browser launch fails after a successful render, printing the error and the surviving rendered path to stderr. mdo-open propagates the child's exit status instead of always reporting success (CREATE_NO_WINDOW preserved on Windows). Watch mode now watches the parent directory and filters events to the target file, so atomic temp-file-rename saves no longer kill the watch. Trailing-edge debounce coalesces save bursts into one render and cannot drop the final event; failed renders keep the loop alive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: run tests on Windows with registry-safe seams (mdo-bs3.1.4) Windows CI now runs the full test suite instead of compile-only checking. Registry install/uninstall is split into a pure plan builder (unit-tested for key/value composition, wrapper branching, and best-effort semantics) and a thin reg.exe executor, so no test touches the real registry. New wrapper integration tests cover argument forwarding and exit-status propagation; setup-launcher argument construction gains metacharacter coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address cross-review findings on open/watch correctness Findings from an independent GPT-5.6 review plus orchestrator review: - Gate Unix-rooted dir_to_file_url tests with cfg(unix); they panicked under the new Windows CI test job because /home/... is not an absolute Windows path. - Debounce deadline is now extended only by events for the watched file, so sibling-file churn cannot postpone rendering indefinitely. - Register the directory watch before the initial render so a save landing during that render is queued instead of lost (also removes the readiness race in the watch tests). - Treat an opener that spawns but exits nonzero within 2s as a launch failure (xdg-open with no browser handler, macOS open with no association) instead of reporting success on spawn alone. - Document the symlinked-input watch limitation; strengthen the sibling-change test with an mtime assertion and add coverage for the failing-opener path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 5fc396c commit 3640966

10 files changed

Lines changed: 1174 additions & 123 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ jobs:
5454
run: cargo package --locked
5555

5656
windows-check:
57-
name: Windows compile check
57+
name: Windows build and test
5858
runs-on: windows-latest
5959

6060
steps:
@@ -70,5 +70,10 @@ jobs:
7070
# Swatinem/rust-cache v2
7171
uses: Swatinem/rust-cache@42dc69e1aa15d09112580998cf2ef0119e2e91ae
7272

73-
- name: Check Windows build
74-
run: cargo check --all-targets --all-features --locked
73+
# Building the test binaries exercises every cfg(windows) code path
74+
# (including cfg(test) modules), and running them covers registry
75+
# command construction, wrapper argument forwarding/error propagation,
76+
# and setup-launcher argument construction with pure, seam-based unit
77+
# tests that never touch the real registry or spawn a real terminal.
78+
- name: Run Windows tests
79+
run: cargo test --all-targets --all-features --locked

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ libc = "0.2"
5151
pulldown-cmark = "0.10"
5252
clap = { version = "4", features = ["derive"] }
5353
notify = "6"
54+
url = "2.5"
5455

5556
[build-dependencies]
5657
winresource = "0.1.31"

‎src/bin/mdo-open.rs‎

Lines changed: 63 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,16 @@
1616
//! `CREATE_NO_WINDOW` so the child never gets a console allocated.
1717
//! 3. If launched directly with no file args, opens onboarding: Windows
1818
//! starts the terminal setup in Windows Terminal when available; Linux
19-
//! opens `mdo-setup` for native first-run onboarding.
20-
//! 4. Exits immediately. The child runs detached and pops the browser.
19+
//! opens `mdo-setup` for native first-run onboarding. This step is
20+
//! fire-and-forget (there is nothing meaningful to wait on: the
21+
//! interactive setup runs in a separately-launched window).
22+
//! 4. For the `--open` case, waits for the `mdo`/`mdo-open` child to exit
23+
//! and mirrors its exit code. `CREATE_NO_WINDOW` (Windows) means the
24+
//! child never gets a console regardless of whether we wait on it, so
25+
//! waiting does not reintroduce the flash; it just lets a real render
26+
//! or browser-launch failure surface as a non-zero exit from
27+
//! `mdo-open` instead of always reporting success. The wait is brief —
28+
//! the child only has to render and hand off to the browser launcher.
2129
//!
2230
//! Net result: registering `mdo-open.exe "%1"` in the Explorer file
2331
//! association gives a flash-free double-click experience without changing
@@ -62,14 +70,26 @@ fn main() -> ExitCode {
6270

6371
#[cfg(target_os = "windows")]
6472
{
65-
let result = if args.is_empty() {
66-
spawn_windows_onboarding(&exe_path)
67-
} else {
68-
spawn_windows_open(&exe_path, args)
69-
};
73+
// Onboarding has no exit status worth propagating: it fires off a
74+
// detached `wt` (or a new console as a fallback) and returns as soon
75+
// as that spawn succeeds, well before the interactive setup inside
76+
// it runs. There is nothing to wait on there.
77+
if args.is_empty() {
78+
return match spawn_windows_onboarding(&exe_path) {
79+
Ok(()) => ExitCode::SUCCESS,
80+
Err(_) => ExitCode::from(1),
81+
};
82+
}
7083

71-
match result {
72-
Ok(()) => ExitCode::SUCCESS,
84+
// The `mdo --open` child is CREATE_NO_WINDOW either way, so waiting
85+
// on it does not reintroduce the console flash this binary exists to
86+
// avoid — no console is ever allocated for it, waited-on or not. It
87+
// also exits quickly (render + browser launch), so this keeps
88+
// `mdo-open.exe` responsive while letting a real failure (bad
89+
// Markdown path, no browser handler, ...) surface as a non-zero exit
90+
// instead of always reporting success.
91+
match spawn_windows_open(&exe_path, args) {
92+
Ok(status) => exit_code_from_status(status),
7393
Err(_) => ExitCode::from(1),
7494
}
7595
}
@@ -98,15 +118,39 @@ fn main() -> ExitCode {
98118
cmd
99119
};
100120

101-
match cmd.spawn() {
102-
Ok(_) => ExitCode::SUCCESS,
121+
// Non-Windows binaries never had a console-flash problem to begin
122+
// with (that's Windows Explorer + console-subsystem-exe behavior),
123+
// so there's no reason to stay fire-and-forget here: wait for the
124+
// child (either `mdo --open`, which exits quickly, or `mdo-setup`,
125+
// which itself only blocks when it decides to run onboarding inline
126+
// in an already-attached terminal) and propagate its exit status.
127+
match cmd.status() {
128+
Ok(status) => exit_code_from_status(status),
103129
Err(_) => ExitCode::from(1),
104130
}
105131
}
106132
}
107133

134+
/// Map a child's exit status onto an `ExitCode`. Signals/unknown termination
135+
/// (no exit code available) fall back to a generic failure code rather than
136+
/// panicking or silently reporting success. Windows exit codes are 32-bit, so
137+
/// a failure whose low byte is zero (e.g. 0x100) must not truncate to
138+
/// "success"; any failing status maps to a non-zero code.
139+
fn exit_code_from_status(status: std::process::ExitStatus) -> ExitCode {
140+
if status.success() {
141+
return ExitCode::SUCCESS;
142+
}
143+
match status.code() {
144+
Some(code) if code as u8 != 0 => ExitCode::from(code as u8),
145+
_ => ExitCode::FAILURE,
146+
}
147+
}
148+
108149
#[cfg(target_os = "windows")]
109-
fn spawn_windows_open(exe_dir: &Path, args: Vec<std::ffi::OsString>) -> io::Result<()> {
150+
fn spawn_windows_open(
151+
exe_dir: &Path,
152+
args: Vec<std::ffi::OsString>,
153+
) -> io::Result<std::process::ExitStatus> {
110154
let mut cmd = Command::new(exe_dir.join(MDO_BIN));
111155
cmd.arg("--open");
112156
cmd.args(args);
@@ -130,14 +174,18 @@ fn spawn_windows_onboarding(exe_dir: &Path) -> io::Result<()> {
130174
}
131175

132176
#[cfg(target_os = "windows")]
133-
fn spawn_without_console(mut cmd: Command) -> io::Result<()> {
177+
fn spawn_without_console(mut cmd: Command) -> io::Result<std::process::ExitStatus> {
134178
use std::os::windows::process::CommandExt;
135179

136180
// CREATE_NO_WINDOW (0x0800_0000) tells Windows not to give the child a
137181
// console of its own. Without it, even though *we* have no console, the
138182
// child (a console-subsystem exe) would briefly get one allocated —
139-
// exactly the flash we are trying to avoid.
183+
// exactly the flash we are trying to avoid. This holds whether we wait
184+
// on the child afterwards (as we now do, to propagate its exit status)
185+
// or not: the flag controls console allocation at spawn time, not
186+
// whether the parent waits.
140187
const CREATE_NO_WINDOW: u32 = 0x0800_0000;
141188
cmd.creation_flags(CREATE_NO_WINDOW);
142-
cmd.spawn().map(|_| ())
189+
let mut child = cmd.spawn()?;
190+
child.wait()
143191
}

0 commit comments

Comments
 (0)