From d8a85b602c869cfea1154a18d0bbacbdfdbd550e Mon Sep 17 00:00:00 2001 From: onevcat Date: Mon, 27 Apr 2026 09:38:59 +0900 Subject: [PATCH] fix(repo-appearance): seed appearance synchronously and curate presets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two user-facing bugs and one tooling-quality fix. **Bug 1+2 — appearance race on Settings reopen.** Previously appearance was loaded from `@Shared(.repositoryAppearances)` inside `RepositorySettingsFeature.task`, which sits behind an async `gitClient.isBareRepository` await for git repos. The Settings UI rendered with `.empty` appearance during that window, so: - clicking a color before the load completed wrote `{icon: nil, color: x}` and wiped the previously-saved icon (Bug 1) - the color picker briefly showed "No color" selected for a repo that did have a saved color (Bug 2) Fix: read `@Shared(.repositoryAppearances)[repository.id]` synchronously in `AppFeature` when constructing the `RepositorySettingsFeature.State` — the same pattern settings/userSettings already use. The State is correct from frame zero, eliminating the race. Removed the redundant `appearanceLoaded` send from `.task` since there's no scenario where appearance changes externally to this Settings window (unlike settings, which can be touched from other windows during the git probe). Two new regression tests pin the behavior: - `selectingRepositorySeedsAppearanceSynchronously` (AppFeature) — selecting a repo whose appearance is in @Shared produces a State with that appearance, no subsequent action required. - `pickingColorKeepsExistingIcon` / `pickingIconKeepsExistingColor` (Reducer) — partial mutations preserve the other field. **Bug 3 — `rocket.fill` rendered as a blank tile.** SF Symbols ships `rocket` but no `.fill` variant; the picker showed an invisible cell. Curate the preset list to symbols verified available since SF Symbols 1–2 (macOS 11–12) baseline. New `RepositoryIconPresetsTests` runs every entry through `NSImage(systemSymbolName:)` so a future bad name is caught at CI rather than in the picker. **Bug 4 — picker had 36 entries, leaving the bottom row half-empty.** Curated list now has exactly 40, organized: folders/containers (8) → docs/books (6) → tags/markers (3) → tools/dev (8) → network (4) → art (2) → nature/symbols (9). Pinned with a count test. --- supacode/Domain/RepositoryIconPresets.swift | 57 ++++++++++++------- .../Features/App/Reducer/AppFeature.swift | 4 +- .../Reducer/RepositorySettingsFeature.swift | 9 --- .../AppFeatureSettingsSelectionTests.swift | 56 ++++++++++++++++++ .../RepositoryIconPresetsTests.swift | 37 ++++++++++++ .../RepositorySettingsAppearanceTests.swift | 37 ++++++++++++ 6 files changed, 171 insertions(+), 29 deletions(-) create mode 100644 supacodeTests/RepositoryIconPresetsTests.swift diff --git a/supacode/Domain/RepositoryIconPresets.swift b/supacode/Domain/RepositoryIconPresets.swift index b8526b4d..e6900708 100644 --- a/supacode/Domain/RepositoryIconPresets.swift +++ b/supacode/Domain/RepositoryIconPresets.swift @@ -6,47 +6,66 @@ import Foundation /// picker ships. The split exists because the same picker is used in /// two contexts that reach for different vocabulary: a tab picker /// favors `play.fill` / `terminal` / `ladybug.fill`, a repo picker -/// favors `folder.fill` / `cube.fill` / `book.fill`. +/// favors `folder.fill` / `book.fill` / `hammer.fill`. /// /// Order is loosely thematic so scanning the grid surfaces intent: -/// folders → boxes/data → tools → web/server → tech → ornament. +/// folders → boxes / data → docs / books → tools / dev → network → +/// art → nature / vibes. +/// +/// All entries are restricted to SF Symbols 1–2 (macOS 11–12) baseline +/// so they're guaranteed-available on the project's macOS 26 minimum. +/// `.fill` variants are only listed when the symbol actually has one +/// (e.g. `rocket.fill` was tried but doesn't exist — only `rocket` +/// does, which renders as a question-mark placeholder if mistakenly +/// suffixed). nonisolated enum RepositoryIconPresets { static let presets: [String] = [ + // Folders / containers (8) "folder.fill", "folder", - "folder.badge.gearshape", + "folder.badge.plus", + "tray.fill", "tray.full.fill", - "tray.2.fill", "shippingbox.fill", - "cube.fill", - "cube.transparent", - "doc.text.fill", + "archivebox.fill", + "externaldrive.fill", + // Docs / books (6) "doc.fill", + "doc.text.fill", + "doc.richtext.fill", "book.fill", "books.vertical.fill", + "bookmark.fill", + // Tags / markers (3) + "tag.fill", + "flag.fill", + "paperplane.fill", + // Tools / dev (8) "hammer.fill", + "wrench.fill", "wrench.and.screwdriver.fill", "screwdriver.fill", - "paintpalette.fill", - "paintbrush.fill", + "gearshape.fill", + "gear", + "cpu", + "ladybug.fill", + // Network / web (4) "globe", "network", "server.rack", "cloud.fill", - "cpu", - "gearshape.fill", - "swift", - "ladybug.fill", - "leaf.fill", + // Art (2) + "paintpalette.fill", + "paintbrush.fill", + // Nature / symbols (9) "star.fill", "heart.fill", + "leaf.fill", "bolt.fill", "sparkles", "flame.fill", - "rocket.fill", - "tag.fill", - "bookmark.fill", - "flag.fill", - "circle.hexagongrid.fill", + "sun.max.fill", + "moon.fill", + "envelope.fill", ] } diff --git a/supacode/Features/App/Reducer/AppFeature.swift b/supacode/Features/App/Reducer/AppFeature.swift index 022f795d..6b315f68 100644 --- a/supacode/Features/App/Reducer/AppFeature.swift +++ b/supacode/Features/App/Reducer/AppFeature.swift @@ -391,12 +391,14 @@ struct AppFeature { } @Shared(.repositorySettings(repository.rootURL)) var repositorySettings @Shared(.userRepositorySettings(repository.rootURL)) var userRepositorySettings + @Shared(.repositoryAppearances) var repositoryAppearances var repoSettingsState = RepositorySettingsFeature.State( rootURL: repository.rootURL, repositoryID: repository.id, repositoryKind: repository.kind, settings: repositorySettings, - userSettings: userRepositorySettings + userSettings: userRepositorySettings, + appearance: repositoryAppearances[repository.id] ?? .empty ) repoSettingsState.globalCopyIgnoredOnWorktreeCreate = state.settings.copyIgnoredOnWorktreeCreate repoSettingsState.globalCopyUntrackedOnWorktreeCreate = state.settings.copyUntrackedOnWorktreeCreate diff --git a/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift b/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift index fbc08985..8f21ccde 100644 --- a/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift +++ b/supacode/Features/RepositorySettings/Reducer/RepositorySettingsFeature.swift @@ -107,13 +107,11 @@ struct RepositorySettingsFeature { switch action { case .task: let rootURL = state.rootURL - let repositoryID = state.repositoryID guard state.capabilities.supportsRepositoryGitSettings else { return .run { send in @Shared(.repositorySettings(rootURL)) var repositorySettings @Shared(.userRepositorySettings(rootURL)) var userRepositorySettings @Shared(.settingsFile) var settingsFile - @Shared(.repositoryAppearances) var appearances let global = settingsFile.global await send( .settingsLoaded( @@ -127,9 +125,6 @@ struct RepositorySettingsFeature { keybindingUserOverrides: global.keybindingUserOverrides ) ) - if let appearance = appearances[repositoryID], !appearance.isEmpty { - await send(.appearanceLoaded(appearance)) - } } } let gitClient = gitClient @@ -138,7 +133,6 @@ struct RepositorySettingsFeature { @Shared(.repositorySettings(rootURL)) var repositorySettings @Shared(.userRepositorySettings(rootURL)) var userRepositorySettings @Shared(.settingsFile) var settingsFile - @Shared(.repositoryAppearances) var appearances let global = settingsFile.global await send( .settingsLoaded( @@ -152,9 +146,6 @@ struct RepositorySettingsFeature { keybindingUserOverrides: global.keybindingUserOverrides ) ) - if let appearance = appearances[repositoryID], !appearance.isEmpty { - await send(.appearanceLoaded(appearance)) - } let branches: [String] do { branches = try await gitClient.branchRefs(rootURL) diff --git a/supacodeTests/AppFeatureSettingsSelectionTests.swift b/supacodeTests/AppFeatureSettingsSelectionTests.swift index 0b680943..b0f73903 100644 --- a/supacodeTests/AppFeatureSettingsSelectionTests.swift +++ b/supacodeTests/AppFeatureSettingsSelectionTests.swift @@ -1,5 +1,8 @@ import ComposableArchitecture +import Dependencies +import DependenciesTestSupport import Foundation +import Sharing import Testing @testable import supacode @@ -79,6 +82,59 @@ struct AppFeatureSettingsSelectionTests { } } + @Test(.dependencies) func selectingRepositorySeedsAppearanceSynchronously() async { + // Regression: selecting a repo whose appearance is already in + // @Shared used to construct a State with `.empty` appearance and + // load asynchronously via .task. The async hop raced with the + // user's first click, sometimes wiping previously-saved fields. + // The State must now carry the appearance from frame zero. + let storage = SettingsTestStorage() + let appearancesURL = URL(fileURLWithPath: "/tmp/appearances-\(UUID().uuidString).json") + let savedAppearance = RepositoryAppearance( + icon: .sfSymbol("hammer.fill"), color: .blue + ) + let repository = Repository( + id: "appearance-repo", + rootURL: URL(fileURLWithPath: "/tmp/appearance-repo"), + name: "AppearanceRepo", + worktrees: [] + ) + + await withDependencies { + $0.settingsFileStorage = storage.storage + $0.repositoryAppearancesFileURL = appearancesURL + } operation: { + @Shared(.repositoryAppearances) var appearances + $appearances.withLock { + $0[repository.id] = savedAppearance + } + + let store = TestStore( + initialState: AppFeature.State( + repositories: RepositoriesFeature.State(repositories: [repository]), + settings: SettingsFeature.State() + ) + ) { + AppFeature() + } withDependencies: { + $0.settingsFileStorage = storage.storage + $0.repositoryAppearancesFileURL = appearancesURL + } + + await store.send(.settings(.setSelection(.repository(repository.id)))) { + $0.settings.selection = .repository(repository.id) + $0.settings.repositorySettings = RepositorySettingsFeature.State( + rootURL: repository.rootURL, + repositoryID: repository.id, + repositoryKind: repository.kind, + settings: .default, + userSettings: .default, + appearance: savedAppearance + ) + } + } + } + @Test func selectingNonRepositoryClearsRepositorySettingsState() async { let repository = Repository( id: "repo-id", diff --git a/supacodeTests/RepositoryIconPresetsTests.swift b/supacodeTests/RepositoryIconPresetsTests.swift new file mode 100644 index 00000000..7eeb1a43 --- /dev/null +++ b/supacodeTests/RepositoryIconPresetsTests.swift @@ -0,0 +1,37 @@ +import AppKit +import Testing + +@testable import supacode + +struct RepositoryIconPresetsTests { + @Test func presetsCountIsForty() { + // Picker grid is 8 columns wide and we want full rows. Pinning + // the count here catches accidental drops or duplicates during + // refactors. + #expect(RepositoryIconPresets.presets.count == 40) + } + + @Test func presetsAreUnique() { + let unique = Set(RepositoryIconPresets.presets) + #expect(unique.count == RepositoryIconPresets.presets.count) + } + + @Test func presetsHaveNoEmptyEntries() { + for symbol in RepositoryIconPresets.presets { + #expect(!symbol.isEmpty) + } + } + + @Test func everyPresetResolvesToARealSFSymbolOnThisOS() { + // Catches a regression where a preset is added with a name like + // `rocket.fill` that *looks* plausible but doesn't actually exist + // (only `rocket` does, no `.fill` variant). A missing symbol + // would render as a blank tile in the picker grid — invisible + // bug. Run on the test machine's macOS, which is at least the + // project's minimum (macOS 26+ per CLAUDE.md). + let missing = + RepositoryIconPresets.presets + .filter { NSImage(systemSymbolName: $0, accessibilityDescription: nil) == nil } + #expect(missing.isEmpty, "Unrecognized SF Symbols in presets: \(missing)") + } +} diff --git a/supacodeTests/RepositorySettingsAppearanceTests.swift b/supacodeTests/RepositorySettingsAppearanceTests.swift index 8e17f1ae..6cb75f17 100644 --- a/supacodeTests/RepositorySettingsAppearanceTests.swift +++ b/supacodeTests/RepositorySettingsAppearanceTests.swift @@ -268,6 +268,43 @@ struct RepositorySettingsAppearanceTests { } } + // MARK: - Regression + + @Test func pickingColorKeepsExistingIcon() async throws { + // Regression: appearance used to be loaded via `.task` async, which + // raced with the first click after reopening Settings — picking a + // color before `.task` finished would write `{icon: nil, color: x}` + // and wipe the previously-saved icon. The fix seeds appearance + // synchronously when the State is built, so the user's first click + // sees the right baseline. This test pins the new behavior: with + // an icon pre-set in initial state, setting a color must preserve + // the icon. + let store = makeStore( + initialAppearance: RepositoryAppearance(icon: .sfSymbol("hammer.fill"), color: nil) + ) + + await store.send(.setAppearanceColor(.blue)) { + $0.appearance.color = .blue + } + + #expect(store.state.appearance.icon == .sfSymbol("hammer.fill")) + #expect(store.state.appearance.color == .blue) + } + + @Test func pickingIconKeepsExistingColor() async throws { + // Mirror of the above for the symmetric case. + let store = makeStore( + initialAppearance: RepositoryAppearance(icon: nil, color: .red) + ) + + await store.send(.setAppearanceIcon(.sfSymbol("folder.fill"))) { + $0.appearance.icon = .sfSymbol("folder.fill") + } + + #expect(store.state.appearance.color == .red) + #expect(store.state.appearance.icon == .sfSymbol("folder.fill")) + } + // MARK: - Persistence helpers private func readAppearances( -- 2.51.2