ReviewOS

craft-native/craft

Multi-window: runtime window creation, and the singleton assumptions in the way

#67
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Direction issue rather than a near-term ask. Today a craft app is exactly one window:

  • No create/open action in bridge_window.zig:55-111; window.craft.window is implicitly "the" window, no handle anywhere.
  • api.zig:199/254 can create windows from zig, but nothing routes that to JS or the CLI.
  • Every bridge is a process-global singleton (macos.zig:4074 setupBridgeHandlers), and outbound JS resolves one global webview — every reply and event in the process is evaluated into a single page regardless of origin.
  • The single NSWindowDelegate instance is process-global and re-attached to each new window (macos_window_events.zig:59-63).
  • window.open() is separately dead (#64).

The eventual want (stacksjs/harness: "pop this session out into its own window") needs: craft.window.create({url,...}) → handle, window-handle addressing on the window namespace, per-webview reply routing, and per-window event scoping. Sequencing: request-id correlation (#66) is a hard prerequisite — "which call asked" must be solved before "which window asked" — and #63's app delegate is where multi-window lifecycle naturally lives. Filing now so the singleton pattern stops spreading into new bridges in the meantime.

🤖 Generated with Claude Code

10 days ago

Round 2 (#62, #63, #64, #65, #66) is done, and four of those touch this issue directly. Notes for whoever picks it up, since the sequencing here changed.

The hard prerequisite is met

#66 landed request-id correlation. Every message the page sends now carries an i, and the dispatcher records it in a stack (src/request_context.zig) that bridge_error.zig reads back when it formats a reply — no handler was touched, and no handler can forget to forward it.

That matters for this issue beyond just being unblocked: the shape it introduced is the shape "which window asked" wants. Adding a w alongside i means one more field on the envelope, one more push/pop frame, and the reply formatter reads both. The 267 reply sites stay untouched a second time. Threading window handles through signatures would be the same 39-file mistake.

Also note the stack was needed, not preferred: runModal spins a nested run loop that keeps dispatching, so a scalar was already wrong for one window. Multi-window does not make that worse — it is already handled.

Two singletons were pre-emptively un-singletoned

The issue asks that the pattern stop spreading into new bridges. In round 2 it did not, in the two places it would have:

  • src/webview_recovery.zig keys crash-retry budgets per webview in an 8-entry table, not one global counter. A window in a crash loop cannot spend the budget of a window that is fine — which is precisely the bug that would land the day a second window exists.
  • macos.zig's content_slots (what to reload after a renderer crash) is keyed the same way, for the same reason.

Both are deliberately tables of eight rather than scalars. Neither needs unpicking.

#63's delegate is already multi-window shaped

applicationShouldHandleReopen: iterates [NSApp windows] and orders every craft window front, filtering by isCraftWindow — AppKit keeps offscreen windows for menus and tooltips, and ordering one of those front puts an empty frame on screen. That loop does not assume one window.

One thing to know before writing window teardown: #63 sets releasedWhenClosed = NO on every window, because the default (YES) deallocated the window on close and made reopen a use-after-free. Measured consequence — the window, its WKWebView, the DOM and the JS context all survive a close. That is what makes reopen restore state instead of reloading. When multi-window adds real window destruction, that is the line it has to pair with, and webview_recovery.forget(key) is the matching cleanup.

#64 left the hook in place

handleCreateWebView (webView:createWebViewWithConfiguration:...) is implemented and returns nil, routing the URL to the system browser. When craft.window.create exists, that method is where a real child window gets created — the delegate callback, the navigation action and the window features are all already in hand. It is a one-branch change, not new plumbing.

The remaining singleton surface, with numbers

Measured on main after round 2, so there is something to track against:

count
global_*_bridge declarations in macos.zig51
getGlobalWebView call sites19
evalJS call sites (all resolve the one global webview)80

bridge.evalJS is the real chokepoint: every reply and every event in the process goes through it, and it resolves one webview with no argument saying which. Per-webview reply routing is mostly one function's signature plus the request-context frame carrying the window — the 80 call sites do not each need a decision, because the dispatcher already knows which window it is serving by the time any of them run. That is the same trick #66 used, and it is why #66 was the prerequisite.

What round 2 did not fix

macos_window_events.zig still re-attaches one process-global NSWindowDelegate instance to each window. Untouched — nothing in round 2 needed it, and changing it without per-window event scoping would only move the problem.

#65's notification delegate is also process-global, and emitNotificationEvent goes through bridge.evalJS. So a notification response is delivered to whichever page the global webview resolves to. Correct with one window; it joins the routing work above rather than needing its own.

10 days ago

Continuation checkpoint (final verified head: 7a16e82). The macOS path now has runtime named creation, idempotent typed handles, sender-scoped mutations and reads, addressed content loads/reload recovery, focused-handle lookup, per-window native event delivery, per-window sidebar/material state, safe JSON handle replies, creation-option parity for frameless/transparent/fullscreen/size/movability/background/maximize controls, and retained-handle listener revival after reopen. The work spans c7909a0 through 7a16e82; CI, SBOM, Binary Size, and Benchmark are green on the final head.\n\nKeeping this direction issue open. The remaining contract is not mechanical: true destroy must pair releasedWhenClosed with registry/content/recovery/observer teardown; executeJavaScript needs an asynchronous result bridge that preserves both request and target webview; parent/modal behavior and non-macOS runtime creation need explicit platform semantics; creator-page subscriptions to child events need an ownership model rather than broadcasting events to unrelated pages. The documentation now states the supported macOS handle lifecycle and does not claim those deferred pieces.

10 days ago

Follow-up: 29c1d4f wires native enter-fullscreen and leave-fullscreen lifecycle events through the macOS window delegate and JS bridge, adds typed capabilities for both channels, and registers all nine window event emitters—including the previously omitted minimize and restore channels. Regression coverage was added, and CI, SBOM, Binary Size, and Benchmark all succeeded on the final commit.

10 days ago

Cross-platform checkpoint: #325 merged into main as e0055f65, preserving 27 focused commits. Linux/WebKitGTK and Windows/WebView2 now support runtime-created named windows with stable typed SDK handles, sender-authenticated bridge calls, replies routed to the originating webview, scoped lifecycle events, and native close/reopen cleanup. Both shipped binaries passed GUI smokes for create, addressed reads and controls, concurrent child opens, close, stale-handle rejection, and same-name reopen.

The merged commit passed CI, Mobile E2E, Benchmark, Binary Size, and SBOM. This completes the runtime-creation and per-window routing milestone. Keeping this direction issue open for the separately documented parent/modal semantics, advanced platform-action parity, and application-level event-sink decisions.

10 days ago

Parent/modal checkpoint on direct main: the approved parent-only modality contract is implemented with AppKit sheets/attached children, GTK transient destroy-with-parent windows and scoped sensitivity, and Win32 owned windows with owner-only disabling. Creator-page event ownership stays separate from native parenting. The GUI and installed PKG/DEB/MSI smokes on d1354c7 passed in the macOS, Linux, and Windows jobs of CI run 36875256696; the overall workflow and iOS simulator are still completing. The Linux backend now restores the parent widget prior sensitivity after the final visible modal closes. Keeping #67 open pending the application-level event-sink policy and the remaining focus/z-order acceptance audit, rather than claiming every multi-window decision is settled.

10 days ago

Verified parent/modal checkpoint on direct main at c569ed3: CI run https://github.com/craft-native/craft/actions/runs/36882021864 is fully green, including macOS, Linux, and Windows Zig/GUI and installed PKG/DEB/MSI smokes, iOS/Watch templates, and integration. The Windows GUI smoke now reads process-owned Win32 handles to verify both the modal and non-modal sibling are owned by the selected parent; it confirms the parent alone is disabled while the sibling and main window stay enabled, and restoration on modal hide/reopen/close. SBOM, Benchmark, and Binary Size are green on the same commit. Keeping #67 open for the application-level event-sink policy and remaining focus/stacking acceptance work (especially real Linux window-manager behavior, which Xvfb cannot prove).

10 days ago

Verified Linux focus/stacking checkpoint on direct main at f70a71f: the CI Linux GUI and installed-DEB smokes now run under Openbox on Xvfb and require an unrelated main window, a non-modal sibling, and the modal to take focus in turn while the modal remains visible. Both Linux smokes passed in CI run https://github.com/craft-native/craft/actions/runs/36885004877. The same run finished green on attempt 2: its first Windows attempt timed out spawning the process-scoped PowerShell probe before an assertion; a targeted Windows job rerun passed the GUI and installed-MSI smokes. Mac, iOS/Watch, integration, SDK, lint, SBOM, Benchmark, and Binary Size also passed on this commit. Keeping #67 open pending the application-level event-sink policy (primary-only versus broadcast or per-subscriber delivery).

10 days ago

Verified native Linux parent/stacking checkpoint on direct main at c4c24b6: the Openbox-backed GUI smoke now reads WM_TRANSIENT_FOR and _NET_CLIENT_LIST_STACKING through its launcher-provided X11 display context, requiring both the modal and non-modal child to be transient for and stacked above their selected parent. The Linux GUI and installed-DEB smokes passed in fully green CI run https://github.com/craft-native/craft/actions/runs/36896008519. Mac and Windows GUI/installed-app jobs, iOS/Watch templates, integration, SDK, lint, SBOM, Binary Size, and Benchmark also passed. The first attempt at this probe failed only because its harness called the unsupported Linux shell bridge; c4c24b6 keeps the production bridge unchanged and passes context directly to the test process. Keeping #67 open pending the application-level event-delivery choice.

10 days ago

Direct-main checkpoint at 14ddaae: https://github.com/craft-native/craft/actions/runs/36999448394 completed fully green. The macOS installed-PKG log now explicitly reports that multi-window, parent/modal, child-event ownership, and orphan-adoption checks passed before the unrelated notification permission stage; notification delivery, clipboard, and deep link also passed in this run. Linux installed-DEB and Windows installed-MSI multi-window jobs, iOS/Watch templates, SDK, integration, lint, SBOM, Binary Size, and Benchmark are green. The earlier two macOS authorization failures on 7500e75 are tracked separately in #330; the strict check remains intact. Keeping #67 open pending the product scope choice for app-level asynchronous event delivery and advanced controls.

10 days ago

The multi-window contract requested here is implemented on direct main: runtime child creation and stable named handles, sender-scoped replies, per-window lifecycle events with creator ownership and orphan adoption, parent-only modal behavior, and macOS/Linux/Windows GUI plus installed PKG/DEB/MSI smokes. CI run https://github.com/craft-native/craft/actions/runs/37002595816 is fully green; its macOS installed-app log confirms the multi-window and parent/modal checkpoint. The approved scope split is documented in dbb468a. Application-wide asynchronous event delivery remains primary-page-only for compatibility and is tracked separately in #331; this closure does not claim per-subscriber routing for those sources.

Sign in to comment on this issue.