fix: address post-merge PR 99 feedback and tests#101
Merged
Conversation
…ng regression guards
Contributor
There was a problem hiding this comment.
Pull request overview
This PR follows up on post-merge feedback from PR #99 by tightening concurrency correctness in the embedded SwiftBuddy server code, refactoring the Settings UI around model reload prompts, and adding regression/unit tests for Issue #97 and several server/config behaviors.
Changes:
- Refactors SwiftBuddy’s embedded server startup to avoid updating
@MainActorstate off the main thread. - Adds/updates regression tests for
<think>tag stripping, role mapping, config persistence, context window token counting, and embedded server wire formats. - Refactors
SettingsViewto centralize the “reload model” prompt UI and related model reload logic.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sandbox/test_shape.swift | Removes an unused sandbox test snippet. |
| tests/SwiftLMTests/ThinkingTagStripTests.swift | Updates Issue #97 regression tests to call the real stripThinkingTags helper and expands role-mapping guards. |
| tests/SwiftLMTests/SwiftBuddyServerTests.swift | Adds tests for embedded server response shapes, SSE chunk formatting, CLI command building, JSON escaping, and role mapping. |
| tests/SwiftLMTests/GenerationConfigPersistenceTests.swift | Adds Codable/persistence and request-mapping/override regression tests for GenerationConfig + server parsing behaviors. |
| tests/SwiftBuddyTests/ContextWindowCalculationTests.swift | Adds a regression test for context token counting using token array size vs shape. |
| SwiftBuddy/generate_xcodeproj.py | Adds CLICommandBuilder.swift to the generated Xcode project source list. |
| SwiftBuddy/SwiftBuddy/Views/SettingsView.swift | Refactors reload prompt into a shared view and adds helper state for reload conditions. |
| SwiftBuddy/SwiftBuddy/ViewModels/ServerManager.swift | Runs server startup in a detached task and moves UI state mutations onto MainActor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Mock a scenario where userInput prepares a chat template with large history. | ||
| // We will directly instantiate LMInput and assert on its size. | ||
|
|
||
| let mockTokens = MLXArray(stride: 0, to: 512, by: 1) |
Comment on lines
+134
to
+135
| let freshDefaults = UserDefaults(suiteName: "com.swiftlm.test.fresh.\(UUID().uuidString)")! | ||
| defer { freshDefaults.removePersistentDomain(forName: "com.swiftlm.test.fresh.\(UUID().uuidString)") } |
Comment on lines
+53
to
+67
| func testModelsResponse_FallsBackToLocalWhenNoModelLoaded() throws { | ||
| // When no model is loaded, the handler returns "local" as the fallback. | ||
| // Clients must still receive a valid list structure. | ||
| let body: [String: Any] = [ | ||
| "object": "list", | ||
| "data": [[ | ||
| "id": "local", | ||
| "object": "model", | ||
| "owned_by": "swiftbuddy" | ||
| ]] | ||
| ] | ||
| let data = try JSONSerialization.data(withJSONObject: body) | ||
| let decoded = try XCTUnwrap(JSONSerialization.jsonObject(with: data) as? [String: Any]) | ||
| let modelList = try XCTUnwrap(decoded["data"] as? [[String: Any]]) | ||
| XCTAssertEqual(modelList[0]["id"] as? String, "local") |
Comment on lines
+85
to
+87
| return """ | ||
| data: {"id":"\(id)","object":"chat.completion.chunk","model":"\(modelId)","choices":[{"index":0,"delta":{"role":"assistant","content":"\(escaped)"},"finish_reason":\(finishReasonJSON)}]}\r\n\r\n | ||
| """ |
Comment on lines
+49
to
+51
| private var needsModelReloadForStreamingChange: Bool { | ||
| effectiveStreamExpertsSetting != currentModelIsMoE | ||
| } |
Comment on lines
+954
to
+956
| switch engine.state { | ||
| case .loading(let progress, let stage): | ||
| VStack(alignment: .leading, spacing: 4) { |
| ConsoleLog.shared.error("Server failed: \(error.localizedDescription)") | ||
| self.isOnline = false | ||
| await MainActor.run { | ||
| self.isOnline = false |
…ests build error - InferenceEngine: TurboKV now iterates KV cache layers (not model.modules()) KVCacheSimple is not a Module subclass, so the cast always failed silently. Also switched to MLXLMCommon.generate(input:cache:parameters:context:) so the pre-built cache (with turboQuantEnabled flags) is actually used in generation. - ContextWindowCalculationTests: replace MLXArray(stride:to:by:) with MLXArray(Array(0..<512)) — the stride initializer does not exist in this version of mlx-swift (MLXArray.init(_ value: Int) is the only Int overload).
…nux CI Use MLXArray(Array(0..<512)) instead of MLXArray(stride:to:by:) — the stride initializer does not exist in this mlx-swift version. Previous fix went to the case-insensitive macOS path Tests/ which is the same file locally but different on the case-sensitive Linux runner.
1. GenerationConfigPersistenceTests: capture fresh suite name in variable
so defer{ removePersistentDomain } removes the same suite it created.
2. SwiftBuddyServerTests: align /v1/models fallback test with prod 'none'
(not 'local') — matches ServerManager .ready/default handler.
3. SwiftBuddyServerTests: remove 'role' from SSE delta helper — embedded
ServerManager does not include role in streaming delta objects.
4. SettingsView: add .downloading case to modelReloadPrompt via shared
progressRow() helper (download progress was invisible during model reload).
5. SettingsView: fix needsModelReloadForStreamingChange to track the
stream-experts value at last model load (appliedStreamExperts @State),
preventing false-positive reload prompts for MoE catalog models.
6. SettingsView: capture appliedStreamExperts on .onAppear and on engine
.ready transitions via .onChange(of: engine.state).
7. ServerManager: clear runningConfiguration + restartRequired on startup
failure so the UI doesn't show a stale 'running config' when offline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses Copilot review feedback on ServerManager actor isolation, adds missing tests for Context Window and Config Persistence, adds regression tests for Issue 97, and updates SettingsView.