Skip to content

Fix Desktop Window Lifecycle#52

Merged
dannysmith merged 8 commits into
mainfrom
fix/desktop-window-lifecycle-crash-resilience
Mar 27, 2026
Merged

Fix Desktop Window Lifecycle#52
dannysmith merged 8 commits into
mainfrom
fix/desktop-window-lifecycle-crash-resilience

Conversation

@dannysmith

@dannysmith dannysmith commented Mar 27, 2026

Copy link
Copy Markdown
Owner

Fixes #48

Summary by CodeRabbit

  • New Features

    • Automatic periodic vault rescanning runs in the background to keep content up to date.
  • Bug Fixes

    • Improved window behavior on macOS: closing/hiding and reopen restore/focus work more reliably.
    • Cleaner shutdown: quick-search pane and global shortcuts are now properly cleaned up for more stable exits.
    • More robust vault update handling to reduce missed changes.

dannysmith and others added 7 commits March 26, 2026 18:32
Investigation and phased plan for fixing window close behavior,
file watcher crash resilience, and Screen Time/App Nap issues.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Intercept CloseRequested on macOS to prevent_close + hide the app
- Add RunEvent::Reopen handler to show window on dock icon click
- Move cleanup (hide quick pane, unregister shortcuts) to RunEvent::Exit
  so it runs on actual quit (Cmd+Q, menu Quit) to prevent known crashes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The window-state plugin only auto-restores on startup, not after
a hide/show cycle. Explicitly call restore_state() when reopening
the main window via the dock icon.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use window.hide() instead of app_handle.hide() so the quick pane
can be shown independently without triggering a system-level app
unhide. Cmd+H still hides the whole app via NSApplication as normal.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Document all decisions and fixes from Phase 1 including the
window.hide() vs app_handle.hide() choice, RunEvent::Exit cleanup,
and window state restore on reopen. Includes behavior summary table
for reference when applying the same pattern to other Tauri apps.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Picks up the event deduplication fix (0.7.0) which prevents
duplicate vault-changed events from being emitted. notify 9.x
FSEvents crash fixes are still in RC — they'll arrive when
a future debouncer version picks up stable notify 9.0.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 2: Make the vault file watcher resilient to crashes and missed events.

- Watcher error callback now emits vault-changed to trigger a refresh,
  picking up any events that were lost during the error
- Add periodic vault rescan (every 5 minutes) as a safety net for
  silent watcher death, App Nap suspension, or FSEvents coalescing
- Export VAULT_CHANGED_EVENT constant for use across modules

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Mar 27, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 77ec8d99-60c0-49f1-a364-196451e14643

📥 Commits

Reviewing files that changed from the base of the PR and between fc89bf2 and df701da.

📒 Files selected for processing (1)
  • tdn-desktop/src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
  • tdn-desktop/src-tauri/Cargo.toml

📝 Walkthrough

Walkthrough

Added a periodic background vault rescan, bumped Rust/tooling and a file-watcher dependency, reworked app run-event handling (macOS window close/reopen and cleanup), and exposed VAULT_CHANGED_EVENT publicly for external use.

Changes

Cohort / File(s) Summary
Dependency Update
tdn-desktop/src-tauri/Cargo.toml
Bumped rust-version baseline to 1.85 and upgraded notify-debouncer-full from 0.5 to 0.7.
App Lifecycle & Event Handling
tdn-desktop/src-tauri/src/lib.rs
Added start_periodic_rescan background loop to refresh vaults and emit events; consolidated run-event handling to prevent closing the main window on macOS (hide instead), handle Reopen to restore/focus the main window, and perform cleanup on Exit. Removed separate main-window-close helper.
Vault Manager & Exports
tdn-desktop/src-tauri/src/vault/manager.rs, tdn-desktop/src-tauri/src/vault/mod.rs
Made VAULT_CHANGED_EVENT a pub const and re-exported it from the vault module; changed watcher error iteration to borrow and emit VAULT_CHANGED_EVENT when errors occur, with additional warning logs.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Desktop app UI #24: Modifies the same vault integration and lib.rs run-event/startup areas; likely overlaps with event wiring and vault manager behavior.

Poem

🐇 I hop and I watch the vaults at play,

Rescanning softly through night and day.
A close turns to hiding — then back with a grin,
Shortcuts still hum as the app wakes within.
Happy hops for the changes we bring!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix Desktop Window Lifecycle' directly relates to the core issue being addressed: fixing window lifecycle behavior (preventing window close from making the app unusable, enabling window reopening, maintaining functionality).
Linked Issues check ✅ Passed The implementation addresses all three coding requirements from issue #48: preventing window close from quitting the app (macOS), enabling window reopening via dock click/reopen events, and maintaining global keyboard shortcut functionality when the main window is closed.
Out of Scope Changes check ✅ Passed All changes directly support the window lifecycle fixes: dependency upgrades enable robustness, periodic vault rescanning ensures app health when window is closed, and event handling rework implements the macOS window behavior requirements.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/desktop-window-lifecycle-crash-resilience

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tdn-desktop/src-tauri/src/lib.rs (1)

236-259: Periodic rescan thread lacks graceful shutdown.

The spawned thread runs an infinite loop with no mechanism to stop it on app exit. While this works in practice (the process terminates anyway), it's worth noting:

  1. If vault_manager.refresh() holds a lock while RunEvent::Exit cleanup runs, there could be contention.
  2. Any panic in this thread is silent (no panic handler).

Consider adding an AtomicBool shutdown flag checked in the loop, or using std::thread::park/unpark for cleaner shutdown. This is optional given the simplicity of the current approach.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tdn-desktop/src-tauri/src/lib.rs` around lines 236 - 259, The periodic rescan
thread spawned by start_periodic_rescan runs an infinite loop and lacks a
shutdown mechanism; modify start_periodic_rescan to accept or create a shared
AtomicBool shutdown flag (stored in app state) that the loop checks each
iteration (and after sleeping) and exit the loop when set, and store the
JoinHandle or provide an unpark/unset method on shutdown to set the flag from
the main shutdown handler (e.g., RunEvent::Exit) so VaultManager.refresh and
event emission (vault::VAULT_CHANGED_EVENT) can finish without contention and
the thread can be joined/cleanly terminated; ensure panics are logged by
wrapping the thread body in catch_unwind and logging any panic to avoid silent
failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tdn-desktop/src-tauri/Cargo.toml`:
- Line 48: The dependency notify-debouncer-full@0.7 requires Rust 1.85 while the
project targets Rust 1.82; either update the crate's Rust toolchain requirement
in Cargo.toml by bumping rust-version to 1.85 or pin the notify-debouncer-full
dependency to a 0.5.x release that supports Rust 1.82; locate the
notify-debouncer-full entry in Cargo.toml and change either the rust-version
field to "1.85" or set notify-debouncer-full = "0.5" to restore compatibility.

---

Nitpick comments:
In `@tdn-desktop/src-tauri/src/lib.rs`:
- Around line 236-259: The periodic rescan thread spawned by
start_periodic_rescan runs an infinite loop and lacks a shutdown mechanism;
modify start_periodic_rescan to accept or create a shared AtomicBool shutdown
flag (stored in app state) that the loop checks each iteration (and after
sleeping) and exit the loop when set, and store the JoinHandle or provide an
unpark/unset method on shutdown to set the flag from the main shutdown handler
(e.g., RunEvent::Exit) so VaultManager.refresh and event emission
(vault::VAULT_CHANGED_EVENT) can finish without contention and the thread can be
joined/cleanly terminated; ensure panics are logged by wrapping the thread body
in catch_unwind and logging any panic to avoid silent failures.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3b2e10e9-227b-4423-942a-79f7043dfd2f

📥 Commits

Reviewing files that changed from the base of the PR and between 9de18b4 and fc89bf2.

⛔ Files ignored due to path filters (2)
  • docs/tasks-todo/task-x-desktop-window-lifecycle-and-crash-resilience.md is excluded by !**/*.md
  • tdn-desktop/src-tauri/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • tdn-desktop/src-tauri/Cargo.toml
  • tdn-desktop/src-tauri/src/lib.rs
  • tdn-desktop/src-tauri/src/vault/manager.rs
  • tdn-desktop/src-tauri/src/vault/mod.rs

Comment thread tdn-desktop/src-tauri/Cargo.toml
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dannysmith dannysmith merged commit 55bbf04 into main Mar 27, 2026
4 checks passed
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.

Bug: Closing the desktop app main window renders the app unusable.

1 participant