Fix Settings workspace add/edit sheets silently no-op-ing

[dhilgaertner]

Summary

• Commit 8827cf8 made the Settings window app-modal via NSApp.runModal(for:). AppKit modal sessions take over event dispatch and prevent SwiftUI's .sheet(...) from presenting on the modal-hosted NSWindow — so clicking the pencil or Add buttons in Settings → Workspaces silently set state but never showed the form.
• Drop the modal session and present Settings as a regular key window, mirroring the existing showAbout() pattern in the same file.
• Re-add the existing-window short-circuit (so repeated Cmd+, brings the already-open Settings window forward instead of stacking duplicates), move settingsWindow cleanup into the willClose observer, and store the observer token on AppDelegate so the closure doesn't capture a mutable local var (which Swift 6 flags as a sendability warning).

Closes #236

Test plan

• make build — clean compile, no Sendable warnings
• CrowUI tests pass (22/22)
• Cmd+, → Settings opens; Workspaces tab → click "+ Add" → WorkspaceFormView sheet appears
• Workspaces tab → click pencil on existing workspace → sheet appears, prefilled
• With Settings open, press Cmd+, again → no second window (existing brought to front)
• Close and reopen Settings → fresh window, no stale state
• Open Settings via the sidebar gear icon → same behavior
• While Settings is open, the main window remains interactive (modality is gone)

🤖 Generated with Claude Code

[dgershman]

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

• No new external input vectors introduced. The change is purely window-management plumbing.
• No new IPC, filesystem, or network surface area.

Concerns:

• None identified.

Code Quality

Correctness — looks good. The root cause is well-diagnosed in the PR description: NSApp.runModal(for:) blocks the event loop and prevents SwiftUI .sheet(...) from presenting. Dropping the modal session and switching to a regular key window is the right fix.

Observer lifecycle is handled correctly. The close observer is stored on AppDelegate (avoiding the mutable-local-var Sendability issue), uses [weak self] to avoid retain cycles, and cleans up both settingsWindow and settingsWindowCloseObserver in the willCloseNotification handler. The observer is scoped to object: win, so it won't fire for unrelated window-close notifications.

Duplicate-window guard matches the existing showAbout() pattern — early-return if settingsWindow is non-nil and bring-to-front with makeKeyAndOrderFront(nil).

Minor note (non-blocking): showAbout() (line 568) has no close observer, so aboutWindow is never nilled out. That means a closed About window can't be garbage collected and re-opening it would bring forward a closed window. This is pre-existing and out of scope for this PR, but worth a follow-up if About has the same issue.

Summary Table

Priority	Issue
🟢 Green	Pre-existing: aboutWindow lacks a willCloseNotification observer (not in this PR's scope)

Recommendation: Approve — clean, minimal fix for the modal-blocking bug. No security or correctness concerns.

[dgershman]

Code & Security Review

Security Review

Strengths:

• No security-sensitive changes (no network, IPC, or user-input handling modified)
• The removal of NSApp.runModal(for:) doesn't introduce any new attack surface

Concerns:

• None identified

Code Quality

Correctness:

• The root-cause analysis is correct: NSApp.runModal(for:) blocks the run loop and prevents SwiftUI's .sheet(...) modifier from presenting — dropping the modal session is the right fix.
• The existing-window guard (if let existing = settingsWindow) correctly prevents duplicate windows.
• Nil-ing settingsWindow in the willCloseNotification observer ensures repeated Cmd+, after closing creates a fresh window (not reusing a closed one).

Observer lifecycle:

• The [weak self] capture in the notification closure correctly avoids a retain cycle.
• Removing the observer inside its own callback is safe because the queue: .main parameter means delivery is asynchronous (dispatched), so removal inside the handler won't deadlock NotificationCenter.
• Storing the observer token on self (rather than a local var) resolves the Swift 6 sendability warning mentioned in the PR description.

Minor observation (non-blocking):

• showAbout() doesn't nil aboutWindow on close, so after closing the About window, subsequent calls bring forward the closed (but not deallocated) window. This PR's pattern for Settings is strictly better. Not a concern for this PR, but worth noting for a future cleanup.

Summary Table

Priority	Issue
Green	aboutWindow could adopt the same close-observer pattern in a follow-up

Recommendation: Approve — clean, minimal fix with correct AppKit lifecycle handling. No blocking issues.