Add settings sync mode and collapsible subagent sessions#531
Add settings sync mode and collapsible subagent sessions#531ildunari wants to merge 1 commit intoDimillian:mainfrom
Conversation
- add app_authoritative vs bidirectional settings sync mode and bidirectional merge behavior in backend settings core - add showSubagentSessions setting and sidebar collapse controls for subagent thread children - wire new settings through frontend models/layout and extend tests for settings/thread row behavior Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Pull request overview
Adds a new settings synchronization mode (default app-authoritative, optional bidirectional reconciliation) and introduces sidebar controls to hide sub-agent sessions globally and collapse/expand sub-agent session trees per parent thread.
Changes:
- Add
syncMode(app_authoritative|bidirectional) to settings models and implement bidirectional merge behavior in the Tauri settings core. - Add
showSubagentSessionssetting and wire it through layout/sidebar to hide sub-agent threads. - Add per-parent collapse/expand UI for sub-agent sessions (stateful in the sidebar) and update thread row rendering + styles.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds SettingsSyncMode and new AppSettings fields (showSubagentSessions, syncMode). |
| src/styles/sidebar.css | Adds styles for the per-thread collapse/expand toggle. |
| src/features/settings/hooks/useAppSettings.ts | Adds defaults + normalization for syncMode and default showSubagentSessions. |
| src/features/settings/hooks/useAppSettings.test.ts | Verifies new defaults (syncMode, showSubagentSessions). |
| src/features/settings/components/sections/SettingsDisplaySection.tsx | Adds UI for toggling sub-agent visibility and selecting settings sync mode. |
| src/features/settings/components/SettingsView.test.tsx | Tests new settings UI interactions. |
| src/features/layout/hooks/layoutNodes/types.ts | Plumbs showSubagentSessions through layout node options. |
| src/features/layout/hooks/layoutNodes/buildPrimaryNodes.tsx | Passes showSubagentSessions into the primary layout. |
| src/features/app/hooks/useThreadRows.ts | Adds support for hiding sub-agent rows and per-parent collapsing; exposes hasChildren. |
| src/features/app/hooks/useThreadRows.test.tsx | Adds coverage for hide + collapse behavior. |
| src/features/app/components/WorktreeSection.tsx | Passes show/hide + collapse options to thread row building and thread list rendering. |
| src/features/app/components/WorktreeSection.test.tsx | Updates test render props for new required showSubagentSessions. |
| src/features/app/components/ThreadRow.tsx | Adds collapse toggle button and chevron icon when a thread has children. |
| src/features/app/components/ThreadList.tsx | Threads through hasChildren, collapse state, and toggle handler. |
| src/features/app/components/ThreadList.test.tsx | Updates fixtures for hasChildren. |
| src/features/app/components/Sidebar.tsx | Maintains per-workspace collapsed thread sets; passes show/hide + collapse through pinned + workspace lists. |
| src/features/app/components/Sidebar.test.tsx | Adds test ensuring sub-agent rows can be hidden. |
| src/features/app/components/PinnedThreadList.tsx | Threads through hasChildren, collapse state, and toggle handler for pinned rows. |
| src/features/app/components/PinnedThreadList.test.tsx | Updates fixtures for hasChildren. |
| src/App.tsx | Wires showSubagentSessions to layout and adds phone UX improvements (new chat action + codex tab restore). |
| src-tauri/src/types.rs | Adds backend showSubagentSessions + SettingsSyncMode to Rust settings model with defaults. |
| src-tauri/src/shared/settings_core.rs | Implements sync-mode gated reconciliation and bidirectional merge behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Ok(disk_settings) = read_settings(settings_path) { | ||
| next = merge_bidirectional_settings(previous.clone(), next, disk_settings)?; | ||
| } |
There was a problem hiding this comment.
In bidirectional sync mode, a failure to read the on-disk settings is silently ignored (if let Ok(disk_settings) = read_settings(...)). That means a JSON parse error or other read failure can cause the app to proceed and then overwrite the file with write_settings, potentially discarding a user's manual edits without surfacing any error. Consider propagating the read_settings error (or at least aborting the write) when sync_mode is Bidirectional so users can fix the file instead of losing changes.
| if let Ok(disk_settings) = read_settings(settings_path) { | |
| next = merge_bidirectional_settings(previous.clone(), next, disk_settings)?; | |
| } | |
| let disk_settings = read_settings(settings_path) | |
| .map_err(|e| format!("Failed to read settings for bidirectional sync: {}", e))?; | |
| next = merge_bidirectional_settings(previous.clone(), next, disk_settings)?; |
| pub(crate) async fn get_app_settings_core(app_settings: &Mutex<AppSettings>) -> AppSettings { | ||
| let mut settings = app_settings.lock().await.clone(); | ||
| if let Ok(Some(collaboration_modes_enabled)) = codex_config::read_collaboration_modes_enabled() | ||
| { | ||
| settings.collaboration_modes_enabled = collaboration_modes_enabled; | ||
| } | ||
| if let Ok(Some(steer_enabled)) = codex_config::read_steer_enabled() { | ||
| settings.steer_enabled = steer_enabled; | ||
| } | ||
| if let Ok(Some(unified_exec_enabled)) = codex_config::read_unified_exec_enabled() { | ||
| settings.unified_exec_enabled = unified_exec_enabled; | ||
| } | ||
| if let Ok(Some(apps_enabled)) = codex_config::read_apps_enabled() { | ||
| settings.experimental_apps_enabled = apps_enabled; | ||
| } | ||
| if let Ok(personality) = codex_config::read_personality() { | ||
| settings.personality = personality | ||
| .as_deref() | ||
| .and_then(normalize_personality) | ||
| .unwrap_or("friendly") | ||
| .to_string(); | ||
| if matches!(settings.sync_mode, SettingsSyncMode::Bidirectional) { | ||
| if let Ok(Some(collaboration_modes_enabled)) = codex_config::read_collaboration_modes_enabled() | ||
| { | ||
| settings.collaboration_modes_enabled = collaboration_modes_enabled; | ||
| } | ||
| if let Ok(Some(steer_enabled)) = codex_config::read_steer_enabled() { | ||
| settings.steer_enabled = steer_enabled; | ||
| } | ||
| if let Ok(Some(unified_exec_enabled)) = codex_config::read_unified_exec_enabled() { | ||
| settings.unified_exec_enabled = unified_exec_enabled; | ||
| } | ||
| if let Ok(Some(apps_enabled)) = codex_config::read_apps_enabled() { | ||
| settings.experimental_apps_enabled = apps_enabled; | ||
| } | ||
| if let Ok(personality) = codex_config::read_personality() { | ||
| settings.personality = personality | ||
| .as_deref() | ||
| .and_then(normalize_personality) | ||
| .unwrap_or("friendly") | ||
| .to_string(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR description says existing behavior is preserved unless sync mode is switched to bidirectional, but get_app_settings_core now only pulls values from codex_config when sync_mode is Bidirectional. With the new default (AppAuthoritative), manual edits to the managed Codex config fields will no longer be reflected on reads. If that behavior change is intentional, it may need to be called out explicitly; otherwise consider still reconciling these fields on read in app-authoritative mode (or revising the default).
| if let Ok(disk_settings) = read_settings(settings_path) { | ||
| next = merge_bidirectional_settings(previous.clone(), next, disk_settings)?; | ||
| } | ||
| reconcile_managed_config_fields(&previous, &mut next); |
There was a problem hiding this comment.
merge_bidirectional_settings can also overwrite syncMode itself (since it merges all keys when incoming == previous). Because the if matches!(next.sync_mode, Bidirectional) check happens before the merge, reconcile_managed_config_fields will still run even if the merge changes next.sync_mode to AppAuthoritative based on disk. Consider either (a) excluding syncMode from the merge so the mode is always app-driven, or (b) re-checking next.sync_mode after the merge before running bidirectional-only reconciliation logic.
| reconcile_managed_config_fields(&previous, &mut next); | |
| if matches!(next.sync_mode, SettingsSyncMode::Bidirectional) { | |
| reconcile_managed_config_fields(&previous, &mut next); | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22aaebcc46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let previous = app_settings.lock().await.clone(); | ||
| let mut next = settings; | ||
|
|
||
| if matches!(next.sync_mode, SettingsSyncMode::Bidirectional) { | ||
| if let Ok(disk_settings) = read_settings(settings_path) { |
There was a problem hiding this comment.
This reconciliation path uses previous from the in-memory mutex as the merge baseline, but in bidirectional mode get_app_settings_core overlays Codex-managed values on a clone and does not persist them back into that mutex. If Codex config changes while the settings UI is open, untouched managed fields can be misclassified as user-edited (incoming != previous), which skips reconcile_managed_config_fields and causes the later writes to overwrite newer external config edits. Recomputing the baseline from current persisted/synced values before merge would prevent these lost updates.
Useful? React with 👍 / 👎.
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| onToggleThreadChildren?.(workspaceId, thread.id); | ||
| }} |
There was a problem hiding this comment.
The new collapse chevron stops propagation for mouse clicks, but the parent row still handles Enter/Space in onKeyDown. When a keyboard user activates the chevron, the key event bubbles to the row and also triggers thread selection, so collapsing/expanding unexpectedly opens that thread. The chevron needs keyboard event propagation handling (or the row key handler should ignore descendant targets).
Useful? React with 👍 / 👎.
|
Context from user testing on iOS/macOS sidebar UX:\n\n- Spawned subagent sessions currently always populate under the parent thread, which can create visual noise in large runs.\n- There was no obvious collapse affordance at the parent row level, so users couldn’t quickly hide/show child sessions in place.\n\nThis PR addresses that by:\n1) adding a global setting to show/hide subagent sessions in the sidebar\n2) adding per-parent collapse/expand controls (caret) for subagent children\n3) preserving existing behavior by default unless the user opts in to different settings sync behavior. |
Summary
Testing
Notes