From 41717d0f1ea1a228cc29dd40684aa7eee8a419f3 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Sat, 16 May 2026 23:03:00 +0800 Subject: [PATCH] =?UTF-8?q?fix(macos):=20publish=20Unset=20prefs=20too=20?= =?UTF-8?q?=E2=80=94=20they=20carry=20the=20catalog=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #67 filtered Unset entries at the snapshot→state boundary and defaulted them to unselected in the TUI editor. The reasoning ("Unset means user had no opinion, don't propagate") was wrong: macOS does not write default-equal values to the plist, so common cases like "Show Sound in menu bar" — a key the user has never explicitly touched but which macOS displays by default — capture as Unset. Filtering them dropped 8 of 9 Menu Bar items from publish and the web UI showed only "1", unchecked. Treat Unset as purely informational metadata going forward: - internal/cli/snapshot_import.go — drop the boundary filter; publish every captured pref (Unset entries carry the catalog default, which is the value we'd want to enforce regardless). - internal/ui/snapshot_editor.go — default Unset entries to selected. The `(unset, default = X)` badge in the description stays, so users can see which prefs originated from the catalog default vs the user's explicit plist value, but the default action is to include them. - internal/diff/compare.go — unchanged; diffMacOS already treats Unset as "no opinion" on either side, which remains the right semantic for diff (vs publish, which is about transmitting the configured state). Tests inverted to match (and renamed so the new contract is obvious). --- internal/cli/snapshot_import.go | 25 +++++++++++++------------ internal/cli/snapshot_test.go | 13 ++++++++----- internal/ui/snapshot_editor.go | 9 +++++---- internal/ui/snapshot_editor_test.go | 12 +++++++----- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/internal/cli/snapshot_import.go b/internal/cli/snapshot_import.go index 4c7e3e1..2c658ce 100644 --- a/internal/cli/snapshot_import.go +++ b/internal/cli/snapshot_import.go @@ -204,23 +204,24 @@ func buildImportConfig(edited *snapshot.Snapshot, dryRun bool) *config.Config { // Invalid URLs are silently skipped — validation at push time will catch them. } - // Drop Unset prefs at the boundary — they're informational only and - // must not propagate into state (which feeds both restore via Plan → - // Configure and publish via the remote API). RemoteMacOSPref has no - // Unset field by design: the remote config models "what should be - // enforced", with no place for "user had no opinion here". - cfg.SnapshotMacOS = make([]config.RemoteMacOSPref, 0, len(edited.MacOSPrefs)) - for _, p := range edited.MacOSPrefs { - if p.Unset { - continue - } - cfg.SnapshotMacOS = append(cfg.SnapshotMacOS, config.RemoteMacOSPref{ + // Pass every pref through, including those marked Unset on capture. + // Rationale: catalog defaults exist precisely to encode "what a sane + // macOS looks like" — when a user's machine sits at the system default + // (which macOS leaves out of the plist), `defaults read` errors and + // capture marks the pref Unset with the catalog value as a stand-in. + // Filtering those out at publish made the web UI lose entries like + // "Show Sound in menu bar" for any user who hadn't explicitly written + // the key. Treat Unset purely as informational metadata (still used + // by diff to avoid false-positive Missing/Changed). + cfg.SnapshotMacOS = make([]config.RemoteMacOSPref, len(edited.MacOSPrefs)) + for i, p := range edited.MacOSPrefs { + cfg.SnapshotMacOS[i] = config.RemoteMacOSPref{ Domain: p.Domain, Key: p.Key, Type: p.Type, Value: p.Value, Desc: p.Desc, - }) + } } cfg.SnapshotShellOhMyZsh = edited.Shell.OhMyZsh diff --git a/internal/cli/snapshot_test.go b/internal/cli/snapshot_test.go index 1b341dc..a7a79ae 100644 --- a/internal/cli/snapshot_test.go +++ b/internal/cli/snapshot_test.go @@ -111,7 +111,7 @@ func TestBuildImportConfig_MacOSPrefs(t *testing.T) { assert.Equal(t, "Autohide dock", cfg.SnapshotMacOS[0].Desc) } -func TestBuildImportConfig_MacOSPrefs_DropsUnset(t *testing.T) { +func TestBuildImportConfig_MacOSPrefs_PreservesUnset(t *testing.T) { snap := &snapshot.Snapshot{ MacOSPrefs: []snapshot.MacOSPref{ {Domain: "com.apple.dock", Key: "autohide", Type: "bool", Value: "1", Desc: "set"}, @@ -122,11 +122,14 @@ func TestBuildImportConfig_MacOSPrefs_DropsUnset(t *testing.T) { cfg := buildImportConfig(snap, false) - // Only the two non-Unset prefs propagate into state — the Unset entry - // must not become enforced config. - require.Len(t, cfg.SnapshotMacOS, 2) + // All three prefs propagate. Unset entries carry the catalog default + // (the value we want to enforce anyway), so dropping them here would + // silently lose categories whose keys aren't in the user's plist — + // the exact bug that hid 8/9 Menu Bar items from openboot.dev. + require.Len(t, cfg.SnapshotMacOS, 3) assert.Equal(t, "autohide", cfg.SnapshotMacOS[0].Key) - assert.Equal(t, "ShowPathbar", cfg.SnapshotMacOS[1].Key) + assert.Equal(t, "tilesize", cfg.SnapshotMacOS[1].Key) + assert.Equal(t, "ShowPathbar", cfg.SnapshotMacOS[2].Key) } func TestBuildImportConfig_InvalidDotfilesURL_Skipped(t *testing.T) { diff --git a/internal/ui/snapshot_editor.go b/internal/ui/snapshot_editor.go index 51eefd5..6646412 100644 --- a/internal/ui/snapshot_editor.go +++ b/internal/ui/snapshot_editor.go @@ -119,9 +119,10 @@ func NewSnapshotEditor(snap *snapshot.Snapshot) SnapshotEditorModel { if p.Domain == "" || p.Key == "" { continue } - // Unset prefs default to unselected — the user had no opinion on - // the source machine, so don't ship a default value into their - // state/remote-config unless they explicitly tick it. + // Unset prefs default selected too — Value carries the catalog + // default, which is the value we'd want to enforce anyway. The + // "(unset)" tag in the description is informational, telling the + // user the source machine sits at macOS default for this key. desc := fmt.Sprintf("= %s (%s)", p.Value, p.Desc) if p.Unset { desc = fmt.Sprintf("(unset, default = %s) %s", p.Value, p.Desc) @@ -129,7 +130,7 @@ func NewSnapshotEditor(snap *snapshot.Snapshot) SnapshotEditorModel { prefItems = append(prefItems, editorItem{ name: fmt.Sprintf("%s.%s", p.Domain, p.Key), description: desc, - selected: !p.Unset, + selected: true, itemType: editorItemMacOSPref, }) } diff --git a/internal/ui/snapshot_editor_test.go b/internal/ui/snapshot_editor_test.go index 5077922..8dabe1f 100644 --- a/internal/ui/snapshot_editor_test.go +++ b/internal/ui/snapshot_editor_test.go @@ -100,7 +100,7 @@ func TestNewSnapshotEditorSkipsInvalidMacOSPrefs(t *testing.T) { assert.Equal(t, 3, len(prefTab.items)) } -func TestNewSnapshotEditor_UnsetPrefsDefaultUnselected(t *testing.T) { +func TestNewSnapshotEditor_UnsetPrefsDefaultSelectedWithBadge(t *testing.T) { snap := makeTestSnapshot() snap.MacOSPrefs = []snapshot.MacOSPref{ {Domain: "com.apple.dock", Key: "autohide", Value: "true", Desc: "set pref"}, @@ -111,11 +111,13 @@ func TestNewSnapshotEditor_UnsetPrefsDefaultUnselected(t *testing.T) { prefTab := m.tabs[4] require.Len(t, prefTab.items, 2) - // The order matches insertion order: set pref first, unset second. - assert.True(t, prefTab.items[0].selected, "set pref should default to selected") - assert.False(t, prefTab.items[1].selected, "unset pref should default to unselected") + // Both default selected. The catalog value is what we want to enforce + // regardless of whether the source machine had the key in its plist — + // dropping unset prefs here is what hid Menu Bar items from publish. + assert.True(t, prefTab.items[0].selected, "set pref should default selected") + assert.True(t, prefTab.items[1].selected, "unset pref should default selected too") assert.Contains(t, prefTab.items[1].description, "unset", - "unset pref description should advertise the (unset) marker") + "unset pref keeps the informational (unset) badge in description") } func TestNewSnapshotEditorAllItemsSelected(t *testing.T) {