Skip to content

feat: Add SharedToggle component and reorganize settings tabs (PR #2)#70

Merged
eyelock merged 2 commits into
mainfrom
feat/pr2-shared-toggle-reorganization
Jan 18, 2026
Merged

feat: Add SharedToggle component and reorganize settings tabs (PR #2)#70
eyelock merged 2 commits into
mainfrom
feat/pr2-shared-toggle-reorganization

Conversation

@eyelock
Copy link
Copy Markdown
Owner

@eyelock eyelock commented Jan 18, 2026

Summary

Implements PR #2 from the settings UI improvement plan:

  • Creates SharedToggle component for consistent global/local setting UX
  • Reorganizes General and Data & Security tabs for better user flow
  • Updates security settings with safer defaults (all FALSE)

Changes

New Component: SharedToggle

General Tab Reorganization

New section order:

  1. Terminal (theme, copy on select, default working directory, default backend)
  2. Bin (retention days, empty bin button)
  3. Updates (auto-check, beta releases, check now button)
  4. Language (language picker)
  5. Uninstall (moved from Data & Security)
  6. About (version, build)

Rationale: Puts most frequently used settings at top, moves destructive action to end

Data & Security Tab Reorganization

New section order:

  1. Security (3 toggles: agent prompts, external modifications, OSC clipboard)
  2. Data Storage (data directory with browse button)
  3. Data Protection (BackupSettingsView)

Rationale: Security settings at top as they're critical, followed by data management

Security Settings Updates

  • New: allowTerminalsToRunAgentPrompts (default: false)
  • Renamed: confirmExternalLLMModificationsallowExternalLLMModifications
    • Clearer naming to indicate this is a permission, not just confirmation
  • Changed defaults: All security settings now default to FALSE (more secure)
    • allowTerminalsToRunAgentPrompts: false
    • allowExternalLLMModifications: false
    • allowOscClipboard: false

Implementation Notes

Localization

Proper localization for new strings (allowTerminalsToRunAgentPrompts) will be added in PR #7.
For now, using placeholder string "Allow Terminals to run Agent Prompts".

Expected Warnings

SwiftLint reports file_length (535 lines) and type_body_length (409 lines) violations in SettingsView.swift.
These are expected and will be resolved in PR #7 when we extract sections into dedicated views.

Testing

  • ✅ All builds pass with zero errors
  • ✅ All tests pass (521 tests)
  • ✅ Formatting verified with make format
  • ✅ Only expected length warnings in SettingsView.swift

Next Steps

🤖 Generated with Claude Code

**SharedToggle Component**
- Create reusable toggle that respects global settings
- Show disabled state with optional clickable message
- Support help text and custom disabled messages

**Settings Reorganization**
- Reorganize General tab: Terminal, Bin, Updates, Language, Uninstall, About
- Reorganize Data & Security tab: Security, Data Storage, Data Protection
- Move Uninstall section from Data & Security to General

**Security Settings Updates**
- Add allowTerminalsToRunAgentPrompts global setting (default: false)
- Rename confirmExternalLLMModifications → allowExternalLLMModifications
- Change all security defaults to FALSE (more secure by default)

**Implementation Note**: Proper localization for new strings will be added in PR #7.
Expected warnings: file_length (535 lines) and type_body_length (409 lines) in
SettingsView.swift - will be addressed in PR #7 via view extraction.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Jan 18, 2026

Code Review

Issues Found

1. 🌍 Hardcoded UI Strings Not Localized

Location: Sources/TermQ/Views/SettingsView.swift:315-316

Two hardcoded English strings that should use the Strings localization system:

Toggle("Allow Terminals to run Agent Prompts", isOn: $allowTerminalsToRunAgentPrompts)
    .help("Allow terminals to execute prompts from agent context (global setting)")

Issue: These strings will not be translatable to the 40+ supported languages.

Reference: CLAUDE.md localization requirements requires all UI strings to use the Strings.* pattern (see localization.md).

Pattern: All other settings in this file correctly use Strings.Settings.*:

  • Line 318: Strings.Settings.confirmExternalModifications
  • Line 321: Strings.Settings.allowOscClipboard

Fix: Add these strings to the Strings.Settings enum and localize them.


2. 🌍 Hardcoded Default Parameter in SharedToggle

Location: Sources/TermQ/Views/Components/SharedToggle.swift:34

disabledMessage: String = "Disabled globally",

Issue: This user-facing default parameter value is a hardcoded English string that won't be translated.

Reference: CLAUDE.md localization requirements requires all user-facing strings to be localized.

Fix: Either remove the default and require callers to provide localized strings, or use a localized reference like Strings.Settings.disabledGlobally.


3. 🐛 AppStorage Key Mismatch

Location: Sources/TermQ/Views/SettingsView.swift:43

@AppStorage("allowExternalLLMModifications") private var allowExternalLLMModifications = false

Issue: The PR renames the key from "confirmExternalLLMModifications" to "allowExternalLLMModifications", but other files still use the old key:

  • CardEditorView.swift:37 - uses "confirmExternalLLMModifications"
  • TermQApp.swift:15,21,24,27 - uses "confirmExternalLLMModifications"

Impact: Settings changes in SettingsView will NOT affect the actual behavior in CardEditorView and TermQApp because they're reading from different UserDefaults keys. The UI toggle will appear broken.

Fix: Update CardEditorView.swift and TermQApp.swift to use the new key name, or revert this change and rename in a coordinated way across all files.


4. 🐛 Conflicting Default Values for allowOscClipboard

Location: Sources/TermQ/Views/SettingsView.swift:44

@AppStorage("allowOscClipboard") private var allowOscClipboard = false

Issue: This PR changes the default from true to false, but CardEditorView.swift:36 still has true as the default for the same key.

Impact: The effective default depends on which view loads first, causing unpredictable behavior for a security-related setting.

Fix: Update CardEditorView.swift:36 to also use false as the default value to maintain consistency.


Summary

Found 4 issues:

  • 2 localization violations (hardcoded UI strings)
  • 2 bugs (AppStorage key mismatch and conflicting defaults)

All issues should be addressed before merging to maintain code quality and prevent runtime bugs.

Changes:
- Use two-line layout: label + Browse button on line 1, full-width text field on line 2
- Matches PathInputField visual pattern for consistency
- Full PathInputField component integration deferred to PR #5

Updated implementation plan to include TermQ Data Folder in PR #5 scope.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@eyelock eyelock merged commit e76d7e7 into main Jan 18, 2026
6 checks passed
@eyelock eyelock deleted the feat/pr2-shared-toggle-reorganization branch January 18, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant