Skip to content

PR 1: Correct open and watch behavior (v0.6) - #19

Merged
maphew merged 4 commits into
mainfrom
pr1-open-watch
Jul 16, 2026
Merged

maphew merged 4 commits into
mainfrom
pr1-open-watch

Conversation

@maphew

@maphew maphew commented Jul 15, 2026

Copy link
Copy Markdown
Owner

First of five v0.6 pull requests (Beads epic mdo-bs3, feature mdo-bs3.1). Correctness before presentation: honest --open failures, standards-correct file URLs, rename-resilient watch mode, and Windows CI that actually runs tests.

Changes

Honest browser-launch failures (mdo-bs3.1.1)

  • mdo --open exits 0 only when render AND launch both succeed; on launch failure the error and the surviving rendered path go to stderr.
  • An opener that spawns but exits nonzero within 2s (xdg-open with no browser handler; macOS open with no association) is now a failure — spawning is not opening.
  • mdo-open waits on its child and mirrors the exit status instead of always reporting success. CREATE_NO_WINDOW is preserved, so no console flash returns.

Standards-correct file URLs (mdo-bs3.1.2)

  • dir_to_file_url now delegates to url::Url::from_directory_path (already in the dependency tree via ammonia): correct percent-encoding for spaces, #, %, ?, Unicode, plus Windows drive letters, UNC, and \\?\ prefixes.
  • Paths that can't form a valid file URL omit <base href> instead of emitting a broken one.

Rename-resilient watch mode (mdo-bs3.1.3)

  • Watches the parent directory and filters events to the target file, so atomic editor saves (temp-file rename) no longer kill the watch.
  • Trailing-edge 200ms debounce coalesces save bursts into one render and can't drop the final event; only events for the watched file extend the window, so sibling churn can't starve rendering.
  • Watch registration happens before the initial render, closing the lost-save window; failed renders don't end the loop.

Windows CI runs tests (mdo-bs3.1.4)

  • The windows job upgrades from cargo check to cargo test --all-targets --all-features --locked.
  • Registry install/uninstall split into a pure, unit-tested plan builder + thin reg.exe executor — no test touches the real registry.
  • New wrapper integration tests cover argument forwarding and exit-status propagation.

Review

  • Orchestrated multi-agent implementation with Fable 5 review of every diff; two defects caught and fixed in review (exit-code truncation to 0, debounce starvation).
  • Independent cross-provider review by GPT-5.6 (codex, high reasoning): 7 findings, all triaged; 5 fixed in the final commit, 1 already fixed, 1 accepted as documented limitation (symlinked-input replacement).
  • Local gates: fmt, clippy -D warnings, 51 tests green (watch tests triple-run for flake).
  • Note: the cfg(windows)-gated code compiles only on Windows CI — this PR's Windows job is its first real compile+run, so watch that check.

🤖 Generated with Claude Code

maphew and others added 4 commits July 15, 2026 12:52
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>
…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>
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>
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>
@kilo-code-bot

kilo-code-bot Bot commented Jul 15, 2026 •

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (10 files)
  • .github/workflows/ci.yml
  • Cargo.lock
  • Cargo.toml
  • src/bin/mdo-open.rs
  • src/file_manager.rs
  • src/lib.rs
  • src/main.rs
  • src/windows_setup.rs
  • tests/cli.rs
  • tests/wrapper.rs

Reviewed by hy3:free · Input: 72.3K · Output: 2.5K · Cached: 235.9K

@maphew
maphew merged commit 3640966 into main Jul 16, 2026
3 checks passed
@maphew
maphew deleted the pr1-open-watch branch July 16, 2026 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant