Node settings improvements

[tobyspark]

From #256, here is the follow-on work. I can’t write to that PR, hence this. Documented in #256 and commit messages here.

[vade]

PR #259 — Performance Review: EditorInputFocus.swift & related

Architectural choice: environment closure vs. FocusedValue

The rationale is correct. FocusedValue doesn't cross NSWindow boundaries on macOS; SwiftUI environment values do. Given the popover-in-own-NSWindow constraint, this is the right architectural call.

---

Primary concern: closure equality + self-reinforcing invalidation

The author flagged this themselves in the comment but incorrectly rated it as "tolerable":

// Note: the Environment value is a bare closure. Closures aren't
// Equatable, so each GraphCanvas body re-eval injects a "new"
// closure and SwiftUI invalidates descendant readers. Tolerable
// here (GraphCanvas re-evals infrequently, popover content is cheap).

The claim that GraphCanvas re-evals infrequently is wrong in this context. GraphCanvas.body re-evals any time inputFocus changes — and inputFocus changes precisely when a user focuses a TextField, which calls setEditorInputFocus(.nodeSettings), which mutates
inputFocus. This creates a self-reinforcing loop:

TextField gains focus
→ setEditorInputFocus(.nodeSettings) called
→ GraphCanvas.inputFocus mutates
→ GraphCanvas.body re-evals
→ new closure injected into environment
→ all .setEditorInputFocus readers invalidated
→ ReclaimsNodeSettingsFocusModifier.body re-evals

SwiftUI prevents infinite cycling (focused state didn't change, so .onChange doesn't re-fire), but the unnecessary re-eval of every .setEditorInputFocus reader happens on every focus event. Additionally shouldUpdateConnections = true is now set on
connect/disconnect/rename operations, which also drive GraphCanvas re-evals.

Your intermediate-view concern is partially mitigated by SwiftUI's dependency tracking — views that don't read @Environment(.setEditorInputFocus) are not re-evaluated just because an ancestor injects a new value. However, all views that do read it are
unnecessarily invalidated, including anything using .reclaimsNodeSettingsFocus().

Fix: wrap in an always-equal struct (DismissAction pattern)

The author mentioned this but deferred it. It should be applied now. The pattern is exactly how SwiftUI implements DismissAction, OpenURLAction, etc.:

// EditorInputFocus.swift
public struct EditorInputFocusSetter {
fileprivate let action: @mainactor (FabricEditorInputFocus) -> Void
func callAsFunction(_ focus: FabricEditorInputFocus) { action(focus) }
}

extension EditorInputFocusSetter: Equatable {
// Always equal: GraphCanvas is the permanent owner and the action
// always targets the same instance. Prevents descendant invalidation
// on every GraphCanvas body re-eval.
static func == (lhs: Self, rhs: Self) -> Bool { true }
}

private struct EditorInputFocusSetterKey: EnvironmentKey {
static let defaultValue = EditorInputFocusSetter { _ in }
}

extension EnvironmentValues {
var setEditorInputFocus: EditorInputFocusSetter {
get { self[EditorInputFocusSetterKey.self] }
set { self[EditorInputFocusSetterKey.self] = newValue }
}
}

In GraphCanvas.swift, use a weak capture to avoid a retain cycle:

.environment(.setEditorInputFocus, EditorInputFocusSetter { [weak self] focus in
self?.inputFocus = focus
})

Call sites don't change — EditorInputFocusSetter with callAsFunction means you still call it as setEditorInputFocus(.nodeSettings).

---

Secondary concern: ReclaimsNodeSettingsFocusModifier creates a second @focusstate

Every use of .reclaimsNodeSettingsFocus() adds an independent @focusstate private var focused: Bool inside the modifier, alongside whatever @focusstate the call site already has. The comment says "SwiftUI tolerates multiple .focused modifiers" — this is true,
but it means:

• Two separate FocusState subscribers tracking the same field
• On focus change, both fire .onChange(of:) handlers independently
• If a call site uses the modifier AND has its own @focusstate for commit-on-defocus (like MathExpressionView), the modifier's internal state is redundant

MathExpressionView is correctly not using .reclaimsNodeSettingsFocus() — it handles focus directly and inline because it needs the defocus event for commit(). That's the right call. The other nodes (OSCReceive, Timeline, StringFormatter, StringScanner) apply it
to TextFields without their own FocusState, which is clean.

---

The other changes in this PR

┌──────────────────────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Change │ Assessment │
├──────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ shouldUpdateConnections.toggle() → = true everywhere │ Correct. Toggle was a latent bug where paired ops (rename = remove + add) cancelled silently. │
├──────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ @observable on PortRegistry │ Correct. Automatic observation replaces the brittle toggle pattern. More fine-grained than the old .id(shouldUpdateConnections) rebuild. │
├──────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ MathExpressionView buffer + .task(id:) debounce │ Sound. .task(id:) cancellation is the idiomatic debounce. The buffer-breaks-AppKit-select-all explanation is accurate. Undo coalescing (all keystrokes since last commit → one ⌘Z) is the right UX. │
└──────────────────────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

---

Summary

The architecture is correct. The one real issue is the bare-closure environment value — the author's own analysis identified the problem but underestimated how often GraphCanvas re-evals in the focus path. Apply the always-equal struct wrapper now. Everything
else in the PR is solid.

[vade]

I dont think @observable with private variables does anything? unless you want ordered to be specifically surfaced as public private(set) ?

[vade]

Nice catch(es) on this failure!

[tobyspark]

Nothing like using something in anger.

[vade]

see my main comment / concerns from claude review of this approach