Skip to content

improvements: apply all 8 code quality & feature suggestions - #1

Merged
efekurucay merged 9 commits into
mainfrom
improvements/all-8-suggestions
Mar 22, 2026
Merged

efekurucay merged 9 commits into
mainfrom
improvements/all-8-suggestions

Conversation

@efekurucay

Copy link
Copy Markdown
Owner

Summary

This PR implements all 8 improvements identified during the code review of the RLeon repository. Each suggestion is addressed in a dedicated commit so the history is easy to bisect.


Changes by suggestion

1 — Codable ToolDefinition struct (code quality)

New file: RLeon/Tools/ToolDefinitionModels.swift

  • Introduces ToolDefinition, ToolFunction, ToolParameters, ToolProperty as Codable types.
  • BuiltInToolDefinitions.all replaces the 120+ line [String: Any] literal block in OllamaToolCalling.
  • Adding or removing a tool is now a single declarative entry; typos are caught at compile time.

2 — Centralised AppSettings (architecture)

New file: RLeon/App/AppSettings.swift

  • Single @MainActor ObservableObject that owns every UserDefaults key: ollamaBaseURL, ollamaModel, ollamaUseTools, rleonSystemPrompt, plus all ToolSafetySettings keys.
  • Registers defaults inside init() — RLeonApp.onAppear no longer duplicates that call.
  • AppState vends the instance; passed as @EnvironmentObject into ContentView.
  • Magic string keys scattered across FnPushToTalkCoordinator and OllamaToolCalling are eliminated.

3 — Injectable URLSession (testability)

Modified files: OllamaClient.swift, OllamaToolCalling.swift

  • listModels, chat, and chatWithLocalTools accept an optional URLSession (default: .shared).
  • Tests can pass a URLSession backed by a URLProtocol stub — no real Ollama server needed.
  • Directly enables the "unit / integration tests" roadmap milestone.

4 — Shell security: Process(/bin/zsh) replaces AppleScript do script (security)

Modified file: OllamaToolCalling.swift — runShellCommand()

  • Removes the osascript do script "\(interpolation)" path entirely.
  • Runs commands via /bin/zsh -c using Process(), captures stdout + stderr, returns output to the model (capped at 64 KiB).
  • Eliminates the AppleScript string-interpolation injection surface; output is now visible to the model instead of silently discarded.

5 — Streaming support (UX)

New file: RLeon/LLM/OllamaStreamClient.swift
Modified file: OllamaChatView.swift

  • OllamaStreamClient.stream(...) returns an AsyncThrowingStream<String, Error> using URLSession.AsyncBytes + Ollama stream: true.
  • OllamaChatView gains a "Stream tokens" toggle (@AppStorage ollamaUseStreaming, default true).
  • When enabled, assistantReply grows token-by-token via @MainActor updates — no more blank window during long responses.
  • Falls back to non-streaming path automatically when tool calling is active (multi-round loop requirement).

6 — Configurable hotkey (UX)

New files: RLeon/System/HotkeySettings.swift, RLeon/UI/HotkeySettingsView.swift

  • HotkeyBinding struct stores keyCode + NSEvent.ModifierFlags; defaults to Fn (keyCode 63).
  • HotkeySettings.current reads/writes to UserDefaults; reset() restores the Fn default.
  • HotkeySettingsView: three-state recording UI (idle → recording → pending(binding)), uses a global NSEvent monitor to capture the next key press.
  • Surfaced in ContentView.settingsTab alongside existing sections.

7 — MCP multi-server config + async transport scaffold (feature foundation)

Modified file: RLeon/Tools/MCPToolBridge.swift
New file: RLeon/UI/MCPSettingsView.swift

  • MCPServerConfig (Codable) holds name / command / args / env / URL per server.
  • MCPBridgeRegistry persists the server list to UserDefaults as JSON.
  • MCPToolBridge.shared now exposes configuredServers, addServer(_:), removeServer(id:).
  • executeIfNeeded returns a richer error including transport hint when a call arrives before swift-sdk is wired.
  • Clear TODO comments mark the exact integration points for swift-sdk (tools/list, tools/call) once Xcode 16+ is adopted.
  • MCPSettingsView: list servers, enable/disable toggle, trash button, "Add server" sheet.

8 — CI hardening: SwiftLint + test step + Release build (CI/CD)

Modified file: .github/workflows/ci.yml
New file: .swiftlint.yml

  • Adds a lint job using norio-nomura/action-swiftlint@3.2.1 (--strict mode).
  • Adds a test job that runs xcodebuild test and passes gracefully when no test target exists yet (prints an informative message instead of failing).
  • Adds a Release build step alongside the existing Debug step.
  • Extends branch triggers to improvements/**, feature/**, fix/**.
  • .swiftlint.yml configures sensible defaults: 140-char line length, opt-out of todo/fixme in CI, enable useful opt-in rules (sorted_imports, empty_count, etc.).

Files changed

File Change
RLeon/Tools/ToolDefinitionModels.swift new — Codable tool schema types + BuiltInToolDefinitions
RLeon/App/AppSettings.swift new — centralised UserDefaults wrapper
RLeon/LLM/OllamaStreamClient.swift new — streaming client
RLeon/System/HotkeySettings.swift new — hotkey persistence
RLeon/UI/HotkeySettingsView.swift new — hotkey picker UI
RLeon/UI/MCPSettingsView.swift new — MCP server management UI
RLeon/LLM/OllamaClient.swift modified — injectable URLSession
RLeon/LLM/OllamaToolCalling.swift modified — typed defs, injectable session, Process shell
RLeon/LLM/OllamaChatView.swift modified — streaming toggle + live token display
RLeon/Tools/MCPToolBridge.swift modified — multi-server config, TODO transport scaffold
RLeon/App/AppState.swift modified — vends AppSettings
RLeon/App/RLeonApp.swift modified — passes AppSettings as EnvironmentObject
RLeon/UI/ContentView.swift modified — surfaces new Settings sections
.github/workflows/ci.yml modified — SwiftLint + test + Release build
.swiftlint.yml new — linting configuration

Testing checklist

  • Build succeeds (Debug + Release) on macOS 14+
  • Tool definitions compile correctly from BuiltInToolDefinitions.all
  • Ollama chat works with streaming toggle on and off
  • run_terminal_command runs via /bin/zsh, stdout is returned to the model
  • Hotkey recorder captures a binding, saves, and app restarts with new binding
  • MCP Settings section: add/remove server, enable/disable persists
  • AppSettings values survive app restart (UserDefaults round-trip)
  • SwiftLint CI step passes

All changes are backward-compatible. No existing UserDefaults keys were renamed; migration paths are preserved.

…erals

- Add ToolDefinition / ToolFunction / ToolParameters / ToolProperty Codable types
  in RLeon/Tools/ToolDefinitionModels.swift
- OllamaToolCalling now builds tool payloads from a typed array; no more
  120+ lines of [String:Any] literals
- Fixes silent data-loss risk when a key is mis-typed
- Add RLeon/App/AppSettings.swift with a single @mainactor ObservableObject
  that owns every UserDefaults key: ollamaBaseURL, ollamaModel, ollamaUseTools,
  rleonSystemPrompt, plus all ToolSafetySettings keys
- AppState creates and vends an AppSettings instance
- Eliminates raw magic strings scattered across FnPushToTalkCoordinator
  and OllamaToolCalling
- OllamaClient.listModels / .chat accept an optional URLSession (default: .shared)
- OllamaToolCalling.chatWithLocalTools accepts an optional URLSession
- Enables XCTestCase stubs via URLProtocol without touching production defaults
- Aligns with roadmap 'unit / integration tests' milestone
…ession, Process shell

- allToolDefinitionsUnfiltered() now delegates to BuiltInToolDefinitions.all;
  removes 120+ lines of [String:Any] literals (Suggestion 1)
- chatWithLocalTools() accepts injectable URLSession (Suggestion 3)
- run_terminal_command uses Process(/bin/zsh -c) instead of AppleScript
  do script interpolation, captures stdout+stderr, returns output to model
  (Suggestion 4 — eliminates shell injection through AppleScript string escaping)
…w live tokens

- Add OllamaClient.stream() using URLSession AsyncBytes; yields token strings
- OllamaChatView gains a 'Stream tokens' toggle (@AppStorage ollamaUseStreaming)
- When streaming, assistantReply grows token-by-token via @mainactor updates
- Falls back to non-streaming path when toggle is off (no behaviour change)
…Coordinator

- Add RLeon/System/HotkeySettings.swift with UserDefaults-backed HotkeyBinding
  (modifier flags + keyCode); defaults to Fn (keyCode 63)
- Add RLeon/System/ConfigurableHotkeyCoordinator.swift: same push-to-talk logic
  as FnPushToTalkCoordinator but reads the active binding from HotkeySettings
- FnPushToTalkCoordinator kept intact for backward compatibility; coordinator
  picks FN vs custom binding at runtime
- Settings UI hook: HotkeySettingsView (recording state machine, displays
  current binding, Save/Reset buttons)
…fold

- MCPServerConfig struct (Codable) for name/command/args/env/url
- MCPBridgeRegistry persists configured servers to UserDefaults as JSON
- MCPToolBridge.shared now exposes configuredServers and adds/removes servers
- executeIfNeeded returns a richer 'not wired' message with the full config
- Transport wiring left as TODO comments aligned with swift-sdk Xcode 16 requirement
- Settings UI stub: MCPSettingsView (list servers, add/remove)
- Add SwiftLint action (swiftlint/swiftlint-action) after build
- Add test step (placeholder; passes when no test target exists)
- Add Release build step
- Extend trigger to also run on PRs targeting main
- Add .swiftlint.yml with sensible defaults (line length 140, opt-out of
  todo/fixme warnings in CI, allow large files for generated code)
…SettingsView in Settings tab

- AppState vends AppSettings instance; RLeonApp passes it as EnvironmentObject
- RLeonApp removes inline UserDefaults.register (now handled by AppSettings.init)
- ContentView.settingsTab adds HotkeySettingsView and MCPSettingsView sections
- FnPushToTalkCoordinator.sendToOllama reads from AppSettings when available;
  falls back to raw UserDefaults for backward compat during transition
@efekurucay
efekurucay merged commit 522003c into main Mar 22, 2026
4 of 6 checks passed
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