-
Notifications
You must be signed in to change notification settings - Fork 0
feat(settings): guided MCP onboarding + reset app data #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -643,6 +643,41 @@ impl CodescribeConfig { | |
| msg: error.to_string(), | ||
| }) | ||
| } | ||
|
|
||
| /// Wipe all local codescribe data for a privacy "Reset app data" action. | ||
| /// | ||
| /// Removes the two app-owned data trees — the config / logs / transcription | ||
| /// directory (`~/.codescribe`) and the Application Support store | ||
| /// (`~/Library/Application Support/Codescribe`, i.e. settings.json + thread | ||
| /// history) — sourced from the same runtime path helpers the app reads, so a | ||
| /// `CODESCRIBE_DATA_DIR` override is honored and no `~` is ever hardcoded. | ||
| /// | ||
| /// When `include_keys` is true, also removes every Keychain-backed API key | ||
| /// (`KEYCHAIN_ACCOUNTS`) and clears it from the live process env. Deleting the | ||
| /// data trees clears the `setup_done` sentinel, so the next launch replays the | ||
| /// first-run wizard. TCC / permission grants are deliberately NOT touched — | ||
| /// those are the user's to manage in System Settings. | ||
| /// | ||
| /// UserDefaults (window frames / SwiftUI scene restoration) is a CFPreferences | ||
| /// domain with no core helper; the Swift caller clears it before relaunch. | ||
| pub fn reset_app_data(&self, include_keys: bool) -> Result<(), CsError> { | ||
| for dir in app_data_dirs() { | ||
| remove_dir_all_tolerant(&dir).map_err(|error| CsError::Config { | ||
| msg: format!("failed to remove {}: {error}", dir.display()), | ||
| })?; | ||
| } | ||
| if include_keys { | ||
| for account in KEYCHAIN_ACCOUNTS { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user has signed in with ChatGPT and then chooses Reset app data with key removal, this loop only deletes Useful? React with 👍 / 👎. |
||
| delete_key(account).map_err(|error| CsError::Config { | ||
| msg: format!("failed to remove keychain key {account}: {error}"), | ||
| })?; | ||
| // SAFETY: same single-writer invariant as `set_api_key` / | ||
| // `clear_api_key` — settings writes are serialized on one Swift actor. | ||
| unsafe { std::env::remove_var(account) }; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// Re-seed the live hotkey detector atomics after a settings write so mode | ||
|
|
@@ -663,6 +698,31 @@ fn reload_hotkey_runtime() { | |
| codescribe::os::hotkeys::apply_hotkey_config(&Config::load_without_keychain()); | ||
| } | ||
|
|
||
| /// App-owned data directories a full "Reset app data" wipes: the config / logs / | ||
| /// transcription tree first, then the Application Support store. Both come from | ||
| /// the runtime path helpers (honoring `CODESCRIBE_DATA_DIR`); when an override | ||
| /// collapses them onto one path the duplicate is dropped so we never remove twice. | ||
| fn app_data_dirs() -> Vec<std::path::PathBuf> { | ||
| let config_dir = Config::config_dir(); | ||
| let settings_dir = codescribe_core::config::UserSettings::settings_dir(); | ||
|
Comment on lines
+706
to
+707
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| if settings_dir == config_dir { | ||
| vec![config_dir] | ||
| } else { | ||
| vec![config_dir, settings_dir] | ||
| } | ||
| } | ||
|
|
||
| /// `remove_dir_all` that treats an already-absent directory as success: a reset | ||
| /// must not fail just because a tree (e.g. an Application Support store that was | ||
| /// never created) does not exist yet. | ||
| fn remove_dir_all_tolerant(dir: &std::path::Path) -> std::io::Result<()> { | ||
| match fs::remove_dir_all(dir) { | ||
| Ok(()) => Ok(()), | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), | ||
| Err(error) => Err(error), | ||
| } | ||
| } | ||
|
|
||
| /// Built-in default workspace root when `AGENT_WORKSPACE_ROOTS` is unset. Kept in | ||
| /// sync with the `list_projects` tool default (`app/agent/tools/workspace.rs`). | ||
| const DEFAULT_AGENT_WORKSPACE_ROOT: &str = "~/Git"; | ||
|
|
@@ -778,3 +838,66 @@ fn write_prompt(path: &std::path::Path, content: &str) -> Result<(), CsError> { | |
| fs::write(path, content)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod reset_tests { | ||
| use super::{app_data_dirs, remove_dir_all_tolerant}; | ||
| use serial_test::serial; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
|
||
| /// Unique scratch directory under the OS temp dir (never the real home). | ||
| fn scratch(tag: &str) -> std::path::PathBuf { | ||
| let nanos = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .map(|d| d.as_nanos()) | ||
| .unwrap_or(0); | ||
| std::env::temp_dir().join(format!("cs_reset_{}_{tag}_{nanos}", std::process::id())) | ||
| } | ||
|
|
||
| /// The reset scope must follow the `CODESCRIBE_DATA_DIR` override and wipe | ||
| /// exactly that tree — never anything outside it — and a repeat wipe of an | ||
| /// already-gone tree must succeed (idempotent). `#[serial]`: mutates the global | ||
| /// `CODESCRIBE_DATA_DIR` env var read by the core path helpers. | ||
| #[test] | ||
| #[serial] | ||
| fn reset_scope_follows_data_dir_and_is_idempotent() { | ||
| let root = scratch("scope"); | ||
| std::fs::create_dir_all(root.join("transcriptions")).unwrap(); | ||
| std::fs::write(root.join("settings.json"), b"{}").unwrap(); | ||
| std::fs::write(root.join("transcriptions/a.txt"), b"hi").unwrap(); | ||
| let root_canon = root.canonicalize().unwrap(); | ||
|
|
||
| let previous = std::env::var("CODESCRIBE_DATA_DIR").ok(); | ||
| // SAFETY: single-threaded test body, serialized via `#[serial]`. | ||
| unsafe { std::env::set_var("CODESCRIBE_DATA_DIR", &root) }; | ||
|
|
||
| let dirs = app_data_dirs(); | ||
| assert!(!dirs.is_empty(), "reset must target at least one dir"); | ||
| // Every target resolves to the override root — nothing escapes it. | ||
| for dir in &dirs { | ||
| let resolved = dir.canonicalize().unwrap_or_else(|_| dir.clone()); | ||
| assert_eq!( | ||
| resolved, root_canon, | ||
| "reset target escaped the data dir: {dir:?}" | ||
| ); | ||
| } | ||
|
|
||
| for dir in &dirs { | ||
| remove_dir_all_tolerant(dir).unwrap(); | ||
| } | ||
| assert!(!root_canon.exists(), "data tree survived reset"); | ||
|
|
||
| // Idempotent: wiping an absent tree is a no-op, not an error. | ||
| for dir in &dirs { | ||
| remove_dir_all_tolerant(dir).unwrap(); | ||
| } | ||
|
|
||
| // SAFETY: restore prior env, same serialized single-thread context. | ||
| unsafe { | ||
| match previous { | ||
| Some(value) => std::env::set_var("CODESCRIBE_DATA_DIR", value), | ||
| None => std::env::remove_var("CODESCRIBE_DATA_DIR"), | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,6 +265,9 @@ struct AgenticReadinessStepView: View { | |
| .foregroundStyle(CSColor.textFaint) | ||
| .padding(.top, 4) | ||
| statusCard(rows: mcp.rows) | ||
| } else if !model.mcpSetupDismissed { | ||
| mcpSetupPrompt | ||
|
Comment on lines
+268
to
+269
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| .padding(.top, 4) | ||
| } | ||
|
|
||
| OnboardingButton(title: "Refresh", kind: .secondary) { | ||
|
|
@@ -277,6 +280,41 @@ struct AgenticReadinessStepView: View { | |
| } | ||
| } | ||
|
|
||
| /// Shown on the readiness step when no MCP server is configured yet: a short, | ||
| /// human explainer plus a route into the real setup surface and a no-guilt skip. | ||
| /// Replaces the old dead end where a missing `mcp.json` showed nothing at all. | ||
| private var mcpSetupPrompt: some View { | ||
| VStack(alignment: .leading, spacing: 10) { | ||
| Text("MCP servers (optional)") | ||
| .font(CSFont.mono(10, .semibold)) | ||
| .tracking(0.4) | ||
| .foregroundStyle(CSColor.textFaint) | ||
| Text("MCP servers give the agent extra tools — things like code search, " | ||
| + "PR review, or web search. It's entirely optional: skip it now and " | ||
| + "wire servers any time from Settings › Engine.") | ||
| .font(CSFont.ui(13)) | ||
| .lineSpacing(3) | ||
| .foregroundStyle(CSColor.textMutedAlt) | ||
| .fixedSize(horizontal: false, vertical: true) | ||
| HStack(spacing: 10) { | ||
| OnboardingButton(title: "Set up MCP servers", kind: .primary) { | ||
| model.openMcpSettings() | ||
| } | ||
| OnboardingButton(title: "Skip for now", kind: .secondary) { | ||
| model.dismissMcpSetupPrompt() | ||
| } | ||
| } | ||
| } | ||
| .padding(14) | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| .background( | ||
| RoundedRectangle(cornerRadius: 12, style: .continuous) | ||
| .fill(CSColor.surfaceRaised(0.02))) | ||
| .overlay( | ||
| RoundedRectangle(cornerRadius: 12, style: .continuous) | ||
| .strokeBorder(CSColor.hairline(0.07), lineWidth: 1)) | ||
| } | ||
|
|
||
| private func readinessPill(ready: Bool) -> some View { | ||
| let accent = ready ? CSColor.olive : CSColor.terracotta | ||
| let accentLight = ready ? CSColor.oliveLight : CSColor.terracottaLight | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import AppKit | ||
| import SwiftUI | ||
|
|
||
| // State machine for the first-run wizard: current step index (persisted on every | ||
|
|
@@ -115,6 +116,11 @@ final class OnboardingViewModel: ObservableObject { | |
| @Published private(set) var readiness: CsAgenticReadiness? | ||
| @Published private(set) var mcpStatus: CsMcpStatusReport? | ||
|
|
||
| /// Whether the user dismissed the "set up MCP" prompt shown when no MCP server | ||
| /// is configured. Session-only: skipping keeps the readiness step moving without | ||
| /// implying an error, and re-opening the wizard offers the prompt again. | ||
| @Published private(set) var mcpSetupDismissed = false | ||
|
|
||
| // API-key step state. | ||
| @Published private(set) var providers: [CsProviderOption] = [] | ||
| @Published var selectedProviderId: String | ||
|
|
@@ -211,6 +217,23 @@ final class OnboardingViewModel: ObservableObject { | |
| mcpStatus = agentStatus.mcpStatus() | ||
| } | ||
|
|
||
| /// Route the user to the live MCP management surface (Settings › Engine) via a | ||
| /// one-shot deep-link, then open the standard Settings window. The wizard stays | ||
| /// open behind it, so the user can wire a server and return to continue. | ||
| func openMcpSettings() { | ||
| SettingsDeepLink.pendingSection = .engine | ||
| if !NSApp.sendAction(Selector(("showSettingsWindow:")), to: nil, from: nil) { | ||
| // Older selector name kept as a defensive fallback. | ||
| _ = NSApp.sendAction(Selector(("showPreferencesWindow:")), to: nil, from: nil) | ||
|
Comment on lines
+225
to
+227
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For the setup button path, this relies on the private Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
|
|
||
| /// Dismiss the MCP setup prompt for this session so onboarding proceeds without | ||
| /// implying MCP is required. | ||
| func dismissMcpSetupPrompt() { | ||
| mcpSetupDismissed = true | ||
| } | ||
|
|
||
| private func loadProvidersIfNeeded() { | ||
| guard providers.isEmpty else { return } | ||
| providers = engine.availableProviders() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
includeKeysis enabled and any Keychain delete returns an error (for example the user denies Keychain access), this removes both data directories first and then returns without relaunching or clearing defaults. That leaves the running app in a partially reset state even thoughSettingsViewModel.resetAppDatatreats failures as “nothing is relaunched”; deleting the Keychain entries before the filesystem wipe would avoid losing app data on a later Keychain failure.Useful? React with 👍 / 👎.