diff --git a/scarf/scarf/Core/Services/ProjectDashboardService.swift b/scarf/scarf/Core/Services/ProjectDashboardService.swift index 9893708..2d616c7 100644 --- a/scarf/scarf/Core/Services/ProjectDashboardService.swift +++ b/scarf/scarf/Core/Services/ProjectDashboardService.swift @@ -1,6 +1,8 @@ import Foundation +import os struct ProjectDashboardService: Sendable { + private static let logger = Logger(subsystem: "com.scarf", category: "ProjectDashboardService") let context: ServerContext let transport: any ServerTransport @@ -19,23 +21,28 @@ struct ProjectDashboardService: Sendable { do { return try JSONDecoder().decode(ProjectRegistry.self, from: data) } catch { - print("[Scarf] Failed to decode project registry: \(error.localizedDescription)") + Self.logger.error("Failed to decode project registry: \(error.localizedDescription, privacy: .public)") return ProjectRegistry(projects: []) } } - func saveRegistry(_ registry: ProjectRegistry) { + /// Persist the project registry to `~/.hermes/scarf/projects.json`. + /// + /// **Throws** on every non-success path — the previous version of + /// this method silently swallowed `createDirectory` and `writeFile` + /// failures with `try?`, which meant the installer could return a + /// valid-looking `ProjectEntry` while the registry on disk never + /// received the new row (project would complete install, show a + /// success screen, then be invisible in the sidebar). Callers that + /// want fire-and-forget behaviour can still use `try?`, but the + /// choice is now theirs. + func saveRegistry(_ registry: ProjectRegistry) throws { let dir = context.paths.scarfDir if !transport.fileExists(dir) { - do { - try transport.createDirectory(dir) - } catch { - print("[Scarf] Failed to create scarf directory: \(error.localizedDescription)") - return - } + try transport.createDirectory(dir) } - guard let data = try? JSONEncoder().encode(registry) else { return } - // Pretty-print for readability (agents may read this file) + let data = try JSONEncoder().encode(registry) + // Pretty-print for readability (agents may read this file). let writeData: Data if let pretty = try? JSONSerialization.jsonObject(with: data), let formatted = try? JSONSerialization.data(withJSONObject: pretty, options: [.prettyPrinted, .sortedKeys]) { @@ -43,7 +50,7 @@ struct ProjectDashboardService: Sendable { } else { writeData = data } - try? transport.writeFile(context.paths.projectsRegistry, data: writeData) + try transport.writeFile(context.paths.projectsRegistry, data: writeData) } // MARK: - Dashboard diff --git a/scarf/scarf/Core/Services/ProjectTemplateInstaller.swift b/scarf/scarf/Core/Services/ProjectTemplateInstaller.swift index e0aa1b0..c96e564 100644 --- a/scarf/scarf/Core/Services/ProjectTemplateInstaller.swift +++ b/scarf/scarf/Core/Services/ProjectTemplateInstaller.swift @@ -211,7 +211,13 @@ struct ProjectTemplateInstaller: Sendable { var registry = service.loadRegistry() let entry = ProjectEntry(name: plan.projectRegistryName, path: plan.projectDir) registry.projects.append(entry) - service.saveRegistry(registry) + // Must throw on failure — silent failure here used to make the + // installer return a valid entry while the registry on disk + // never got updated, producing the "install completed but the + // project doesn't show up in the sidebar" bug. If the registry + // write fails, the whole install is surfaced as failed so the + // user can see + address the underlying problem. + try service.saveRegistry(registry) return entry } diff --git a/scarf/scarf/Core/Services/ProjectTemplateUninstaller.swift b/scarf/scarf/Core/Services/ProjectTemplateUninstaller.swift index cfa5d14..93f6830 100644 --- a/scarf/scarf/Core/Services/ProjectTemplateUninstaller.swift +++ b/scarf/scarf/Core/Services/ProjectTemplateUninstaller.swift @@ -206,7 +206,17 @@ struct ProjectTemplateUninstaller: Sendable { let dashboardService = ProjectDashboardService(context: context) var registry = dashboardService.loadRegistry() registry.projects.removeAll { $0.path == plan.project.path } - dashboardService.saveRegistry(registry) + // saveRegistry throws now — log a write failure but don't abort + // the uninstall. Every earlier step already completed (files + // removed, skills removed, cron jobs removed, memory stripped, + // Keychain cleared); failing here leaves a stale registry row + // pointing at a deleted project — cosmetic and easy to fix + // from the sidebar. + do { + try dashboardService.saveRegistry(registry) + } catch { + Self.logger.warning("uninstall couldn't rewrite projects registry: \(error.localizedDescription, privacy: .public)") + } Self.logger.info("uninstalled template \(plan.lock.templateId, privacy: .public) from \(plan.project.path, privacy: .public)") } diff --git a/scarf/scarf/Features/Projects/ViewModels/ProjectsViewModel.swift b/scarf/scarf/Features/Projects/ViewModels/ProjectsViewModel.swift index cd46faf..53bfb14 100644 --- a/scarf/scarf/Features/Projects/ViewModels/ProjectsViewModel.swift +++ b/scarf/scarf/Features/Projects/ViewModels/ProjectsViewModel.swift @@ -1,7 +1,9 @@ import Foundation +import os @Observable final class ProjectsViewModel { + private let logger = Logger(subsystem: "com.scarf", category: "ProjectsViewModel") let context: ServerContext private let service: ProjectDashboardService @@ -39,7 +41,19 @@ final class ProjectsViewModel { guard !registry.projects.contains(where: { $0.name == name }) else { return } let entry = ProjectEntry(name: name, path: path) registry.projects.append(entry) - service.saveRegistry(registry) + // saveRegistry throws now. The VM doesn't currently have a + // surface for user-visible errors (there's no alert/toast in + // the Projects view), so log at error level to the unified + // log and keep the in-memory state consistent with whatever + // landed on disk. If the write fails, the added entry won't + // persist across launches — the user sees it appear + work + // this session, then it's gone at relaunch. Not ideal, but + // matches today's UX and flagged for a proper alert later. + do { + try service.saveRegistry(registry) + } catch { + logger.error("addProject couldn't persist registry: \(error.localizedDescription, privacy: .public)") + } projects = registry.projects selectProject(entry) } @@ -47,7 +61,11 @@ final class ProjectsViewModel { func removeProject(_ project: ProjectEntry) { var registry = service.loadRegistry() registry.projects.removeAll { $0.name == project.name } - service.saveRegistry(registry) + do { + try service.saveRegistry(registry) + } catch { + logger.error("removeProject couldn't persist registry: \(error.localizedDescription, privacy: .public)") + } projects = registry.projects if selectedProject?.name == project.name { selectedProject = nil diff --git a/scarf/scarf/Features/Templates/Views/TemplateConfigSheet.swift b/scarf/scarf/Features/Templates/Views/TemplateConfigSheet.swift index 105b9d9..0fddba5 100644 --- a/scarf/scarf/Features/Templates/Views/TemplateConfigSheet.swift +++ b/scarf/scarf/Features/Templates/Views/TemplateConfigSheet.swift @@ -68,16 +68,26 @@ struct TemplateConfigSheet: View { private var footer: some View { HStack { Button("Cancel") { + // Caller owns dismissal — this view is used both as a + // standalone sheet (ConfigEditorSheet, where the caller + // wants dismissal) AND inlined inside the install sheet + // (TemplateInstallSheet.configureView, where calling + // .dismiss here would tear down the OUTER install sheet + // and abort the flow before .planned is reached). onCancel() - dismiss() } .keyboardShortcut(.cancelAction) Spacer() Button(commitLabel) { if let finalized = viewModel.commit(project: project) { onCommit(finalized) - dismiss() } + // Same dismissal-is-caller's-responsibility rule as + // Cancel — inside the install sheet, onCommit transitions + // stage to .planned and the outer view re-renders to + // show the preview. In the edit sheet, onCommit + // transitions the editor VM and its state machine + // handles dismissal via the success view's Done button. } .keyboardShortcut(.defaultAction) .buttonStyle(.borderedProminent) diff --git a/scarf/scarf/Localizable.xcstrings b/scarf/scarf/Localizable.xcstrings index c2de1fc..61022a8 100644 --- a/scarf/scarf/Localizable.xcstrings +++ b/scarf/scarf/Localizable.xcstrings @@ -49,6 +49,10 @@ }, "(%lld tokens)" : { + }, + "*" : { + "comment" : "A required asterisk.", + "isCommentAutoGenerated" : true }, "/%@" : { @@ -2229,6 +2233,9 @@ "already gone" : { "comment" : "A tag for a file that is already gone (no longer in the template).", "isCommentAutoGenerated" : true + }, + "Also works: %@" : { + }, "API Key" : { "localizations" : { @@ -5024,6 +5031,14 @@ } } }, + "Configuration saved" : { + "comment" : "A title displayed when a configuration is saved.", + "isCommentAutoGenerated" : true + }, + "Configuration…" : { + "comment" : "A contextual menu item that opens a configuration editor for a project.", + "isCommentAutoGenerated" : true + }, "Configure" : { "localizations" : { "de" : { @@ -5064,6 +5079,10 @@ } } }, + "Configure %@" : { + "comment" : "The title of the configuration sheet. The argument is the name of the template.", + "isCommentAutoGenerated" : true + }, "Connect timeout" : { "localizations" : { "de" : { @@ -5304,6 +5323,10 @@ } } }, + "Continue" : { + "comment" : "Button label for continuing with the template configuration.", + "isCommentAutoGenerated" : true + }, "Continue Last Session" : { "localizations" : { "de" : { @@ -5584,6 +5607,10 @@ } } }, + "Couldn't save" : { + "comment" : "A title displayed when a configuration save fails.", + "isCommentAutoGenerated" : true + }, "Create" : { "localizations" : { "de" : { @@ -7657,6 +7684,10 @@ } } }, + "Edit configuration" : { + "comment" : "A button that opens a configuration editor for a project.", + "isCommentAutoGenerated" : true + }, "Edit User Profile" : { "localizations" : { "de" : { @@ -10548,6 +10579,9 @@ } } } + }, + "Internal state inconsistency — please close and re-open." : { + }, "Invalid URL" : { "localizations" : { @@ -11156,6 +11190,10 @@ } } }, + "Loading configuration…" : { + "comment" : "A message displayed while loading the configuration.", + "isCommentAutoGenerated" : true + }, "Loading session…" : { "localizations" : { "de" : { @@ -12665,6 +12703,9 @@ } } } + }, + "No configuration" : { + }, "No credential pools configured" : { "localizations" : { @@ -12910,6 +12951,10 @@ } } }, + "No fields" : { + "comment" : "A label that describes a template with no configuration fields.", + "isCommentAutoGenerated" : true + }, "No headers configured." : { "localizations" : { "de" : { @@ -14258,6 +14303,10 @@ } } }, + "Opens on launch" : { + "comment" : "A tooltip for the star button in the Manage Servers view.", + "isCommentAutoGenerated" : true + }, "Optional" : { "localizations" : { "de" : { @@ -16127,6 +16176,10 @@ } } }, + "Recommended model" : { + "comment" : "A label that indicates a recommended model.", + "isCommentAutoGenerated" : true + }, "Reconnect" : { "localizations" : { "de" : { @@ -18000,6 +18053,14 @@ } } }, + "Saved in Keychain — leave empty to keep the stored value." : { + "comment" : "A message that appears when a user has filled in a secret but has not yet saved it.", + "isCommentAutoGenerated" : true + }, + "Saving…" : { + "comment" : "A label displayed while the configuration is being saved.", + "isCommentAutoGenerated" : true + }, "Scarf" : { }, @@ -18043,6 +18104,10 @@ } } }, + "Scarf doesn't auto-switch your active model. Change it in Settings if you'd like." : { + "comment" : "A description of the warning about not switching models.", + "isCommentAutoGenerated" : true + }, "Scarf never prompts for passphrases. Add your key to ssh-agent in Terminal, then click Retry. If your key isn't `id_ed25519`, swap the path:" : { "localizations" : { "de" : { @@ -19291,6 +19356,10 @@ } } }, + "Set as default — open this server when Scarf launches." : { + "comment" : "A tooltip for the star button in the Manage Servers view.", + "isCommentAutoGenerated" : true + }, "Settings" : { "localizations" : { "de" : { @@ -19694,6 +19763,10 @@ } } }, + "Show while typing" : { + "comment" : "A hint for the user on how to show/hide the secret.", + "isCommentAutoGenerated" : true + }, "Signal integration requires signal-cli (Java-based) installed locally. Link this Mac as a Signal device, then keep the daemon running so hermes can send/receive messages." : { "localizations" : { "de" : { @@ -21538,6 +21611,13 @@ } } } + }, + "This Mac" : { + "comment" : "A description of the local machine.", + "isCommentAutoGenerated" : true + }, + "This project wasn't installed from a schemaful template." : { + }, "This provider has no catalogued models." : { "localizations" : { @@ -21739,6 +21819,10 @@ } } }, + "This template has no configuration fields." : { + "comment" : "A description of a template with no configuration fields.", + "isCommentAutoGenerated" : true + }, "This uploads logs, config (with secrets redacted), and system info to Nous Research support infrastructure. Review the output below before sharing the returned URL." : { "localizations" : { "de" : { @@ -23786,6 +23870,10 @@ }, "Where should this project live?" : { + }, + "Will be saved to the Keychain on commit." : { + "comment" : "A description of a secret field that will be saved to the Keychain on commit.", + "isCommentAutoGenerated" : true }, "Working" : { "localizations" : { diff --git a/scarf/scarfTests/ProjectTemplateTests.swift b/scarf/scarfTests/ProjectTemplateTests.swift index 61a3ded..a79a546 100644 --- a/scarf/scarfTests/ProjectTemplateTests.swift +++ b/scarf/scarfTests/ProjectTemplateTests.swift @@ -768,6 +768,111 @@ import Foundation } } +/// State-machine tests for `TemplateInstallerViewModel`. The install +/// flow's configure step is driven entirely through the VM — the view +/// transitions `.awaitingParentDirectory → .awaitingConfig → .planned` +/// based on `submitConfig(values:)` / `cancelConfig()` calls. If those +/// transitions break, the user lands on the wrong sheet stage (or no +/// sheet at all, as in the v1.1.0 regression where the config sheet's +/// internal `dismiss()` tore down the outer install sheet before +/// submitConfig had a chance to fire). +@Suite(.serialized) @MainActor struct TemplateInstallerViewModelTests { + + @Test func submitConfigStashesValuesAndTransitionsToPlanned() throws { + let vm = TemplateInstallerViewModel(context: .local) + // Seed the VM with an awaiting-config plan (schema-ful). + let plan = try Self.makePlanWithConfigSchema() + vm.plan = plan + vm.stage = .awaitingConfig + + let values: [String: TemplateConfigValue] = [ + "site_url": .string("https://example.com") + ] + vm.submitConfig(values: values) + + // Stage must advance past the configure step, values must land + // on the plan where install() will pick them up. + if case .planned = vm.stage { + // ok + } else { + Issue.record("expected .planned, got \(vm.stage)") + } + #expect(vm.plan?.configValues["site_url"] == .string("https://example.com")) + } + + @Test func cancelConfigReturnsToAwaitingParentDirectory() throws { + let vm = TemplateInstallerViewModel(context: .local) + vm.plan = try Self.makePlanWithConfigSchema() + vm.stage = .awaitingConfig + + vm.cancelConfig() + + if case .awaitingParentDirectory = vm.stage { + // ok — user can re-pick the parent dir or fully cancel + } else { + Issue.record("expected .awaitingParentDirectory, got \(vm.stage)") + } + // Plan is preserved so re-entering the configure step doesn't + // re-run buildPlan. + #expect(vm.plan != nil) + } + + @Test func submitConfigNoOpWhenPlanIsNil() { + let vm = TemplateInstallerViewModel(context: .local) + vm.plan = nil + vm.stage = .awaitingConfig + vm.submitConfig(values: ["k": .string("v")]) + // With no plan, the call should be silent — no crash, stage + // stays where it was. (Defensive guard in submitConfig.) + if case .awaitingConfig = vm.stage { + // ok + } else { + Issue.record("expected stage to remain .awaitingConfig when plan is nil; got \(vm.stage)") + } + } + + // MARK: - Fixture + + /// Build a `TemplateInstallPlan` carrying a single-field config + /// schema. Exists as a local helper rather than a shared one + /// because no other suite needs it. + nonisolated static func makePlanWithConfigSchema() throws -> TemplateInstallPlan { + let schema = TemplateConfigSchema( + fields: [ + .init(key: "site_url", type: .string, label: "Site URL", + description: nil, required: true, placeholder: nil, + defaultValue: nil, options: nil, minLength: nil, + maxLength: nil, pattern: nil, minNumber: nil, + maxNumber: nil, step: nil, itemType: nil, + minItems: nil, maxItems: nil) + ], + modelRecommendation: nil + ) + let manifest = ProjectTemplateServiceTests.sampleManifest( + id: "tester/vm-transitions", + configSchema: schema + ) + let tmp = try ProjectTemplateServiceTests.makeTempDir() + // Not a real bundle dir — we never unzip or install from this + // plan, we only test state transitions that don't touch disk. + return TemplateInstallPlan( + manifest: manifest, + unpackedDir: tmp, + projectDir: tmp + "/project", + projectFiles: [], + skillsNamespaceDir: nil, + skillsFiles: [], + cronJobs: [], + memoryAppendix: nil, + memoryPath: ServerContext.local.paths.memoryMD, + projectRegistryName: "VM Transitions", + configSchema: schema, + configValues: [:], + manifestCachePath: tmp + "/project/.scarf/manifest.json" + ) + } +} + /// Validates every `.scarftemplate` shipped under `templates///` /// in the repo. A template whose manifest, `contents` claim, or file set is /// out of sync will fail here — so shipped templates can't silently rot.