feat: Native macOS Phase 3-B — server screens, process lifecycle, security port - #155
Merged
Conversation
…ts traces next-phase-plan.md marked Phase 2 and 3-A as done and introduced Phase 3-B; the matrix had not been updated since Phase 3-A closed. Also commits the 3-1 spike evidence screenshots referenced by phase3a-spike-results.md and excludes local Instruments .trace captures from git. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e app Task 3-4 of Phase 3-B: model MinecraftServer/ServerTemplate from the TS side (src/renderer/shared/server declaration.ts, src/lib/server-commands.ts) as Swift Codable/Sendable value types (Server, ServerTemplate, ServerStatus, AutoBackupScheduleType, ServersFile), plus a minimal actor-based ServerStore for reading/writing the Native app's own servers.json. No UI or CRUD API surface — that's deferred to later Phase 3-B tasks. JSON keys match the TS camelCase property names exactly, so no custom CodingKeys were needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implements the first real UI screen: a NavigationSplitView shell whose sidebar List shows servers loaded via a new @observable ServerListViewModel wrapping ServerStore. Server gains Identifiable conformance (existing id: String) for stable List/ForEach identity. ServerStore gains a defaultFileURL() factory pointing at Application Support, used by the ViewModel's production init while tests inject a temp-file ServerStore. Main.swift's default (no MCV_SPIKE) path now launches a real windowed app via the new MCVectorApp (App-conforming, WindowGroup { RootView() }) instead of just printing and returning -- this is the first task where the app is actually runnable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 3-6: RootView's detail column showed only a static placeholder. Adds ServerDetailView, a read-only Form-based Inspector showing the selected server's identifying/operational fields (status, version, software, port, memory, path, javaPath), plus a selectedServer computed property on ServerListViewModel that resolves the current selection against the loaded servers list (nil for no selection or a stale id). RootView now shows ServerDetailView when selectedServer is non-nil and falls back to the existing ContentUnavailableView placeholder otherwise. No toolbar or start/stop actions -- that's task 3-7. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a ServerProcessService actor that launches/stops Java server child processes (graceful stop via stdin "stop" with a timeout-then-terminate() fallback, crash/exit detection via a monitoring task, and an AsyncStream of status events), wires it into ServerListViewModel's start/stop methods and event subscription, and adds a Start/Stop toolbar to the server detail view. Resolves the interrupted implementation's @ObservationTracked/deinit isolation conflict by marking the housekeeping processEventTask property @ObservationIgnored and using Swift 6.2's isolated deinit to cancel it, instead of nonisolated(unsafe); also fixes a real bug found while verifying -- the event-subscription Task lacked @Concurrent so it silently inherited @mainactor isolation, making its internal await a no-op (compiler warning). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 3-7 code review flagged a real gap: no test proved the ViewModel's @Concurrent event-subscription Task actually applies ServerProcessService events to servers[].status -- the same class of bug (a missing @Concurrent that silently changed the subscription Task's execution context) was previously caught only by manual code reading, not by any test. Adds three tests using a real ServerProcessService + script-based fixture process (mirroring ServerProcessServiceTests.swift's pattern): startSelectedServer() reaching .online synchronously, a self-crashing process reaching .crashed purely via the event stream (stopSelectedServer() is deliberately never called), and stopSelectedServer() reaching .offline via the event stream. Event propagation is awaited via a bounded poll (20ms interval, 2s timeout) rather than a fixed sleep, to avoid flakiness. Verified empirically: removing @Concurrent from ServerListViewModel.init and re-running these tests did not fail them, because the tests' own polling loop (Task.sleep) yields the MainActor cooperatively, letting the MainActor-inherited subscription Task run interleaved regardless. These tests robustly prove the event-stream wiring and apply()/setStatus() logic are correct end-to-end (they would fail if that wiring broke), but the @Concurrent attribute's effect here is executor placement, not observable correctness, so no black-box behavioral test can specifically discriminate its presence -- that remains a code-review-catchable concern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… (task 3-8) Extends ServerProcessService with a live, line-oriented stdout AsyncStream (FileHandle.bytes-based, @Concurrent reader task so it never pins the actor's executor), and adds ServerLogViewModel/ServerLogView to route that stream through the 3-3 spike's validated LogBatcher+LogLineBuffer pipeline into a ScrollView+LazyVStack log view, wired additively into ServerDetailView below the existing config Form. Applied swift-concurrency skill guidance for the AsyncStream bridge and structured-concurrency cancellation (a withTaskGroup + @mainactor children variant hit a Swift 6.2 region-isolation-checker limitation, so streamLogs() uses a defer-scoped Task instead); applied swiftui-expert-skill guidance for ScrollViewReader auto-scroll via a stable sentinel anchor id, unaffected by LogLineBuffer's front-trimming. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 3-8's code review flagged that ServerProcessService.start() redirected stderr to a Pipe whose read end was never drained. macOS pipe buffers are ~64KB; once full, the child's next write() to stderr blocks, which could hang the Minecraft server itself under heavy console output. Now continuously read-and-discard in the background, same reader-lifecycle pattern as stdoutLines(serverId:). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Builds the NSPanel bridge design confirmed by the Phase 3-A spike into a real, working floating console: FloatingConsolePanel (generic over Content, hosting the real ServerLogView instead of the spike's hardcoded GlassSpikeContent) + FloatingConsolePanelController (show/hide/toggle/dismiss lifecycle) + FloatingConsoleContentView (glass restricted to the functional header only, per the requirements doc's stricter policy vs. the spike's whole-content demo glass). Wired into ServerDetailView's toolbar as a toggle button. The riskiest part: the panel and the inline Console Output section must share one ServerLogViewModel without ever calling streamLogs() twice, since ServerProcessService.stdoutLines() is single-consumer. Solved by hoisting the .task(id: server.status) that drives streamLogs() out of ServerLogView and up into ServerDetailView, which now owns it exclusively -- ServerLogView (used both inline and inside the panel) is now a pure display component that only reads viewModel.lines, safe to instantiate any number of times. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extends ServerListViewModel's existing single subscription to processService.events (the apply(_:) method) to also append a bounded, newest-first activityLog of ActivityEntry values, rather than adding a second AsyncStream consumer -- processService.events is single-consumer, and a competing loop would silently split/steal event delivery from the existing status-tracking subscriber. UI is a standard .inspector panel toggled from RootView's toolbar, listing entries in a plain List; both get automatic Liquid Glass styling with zero manual .glassEffect calls, per swiftui-expert-skill's Inspector guidance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-flake under parallel load This file now has 3 crash-script-based tests instead of 1 (task 3-10's Activity Drawer tests), and Swift Testing parallelizes test execution -- enough concurrent real Process launches under contention occasionally pushed the pre-existing 2s poll timeout past its budget purely from CPU scheduling delay, not a functional bug. waitUntil still returns as soon as its condition is true, so this only affects how long a genuinely-broken test takes to fail, not passing-run duration. Verified stable across 8+ repeated full-suite runs after the change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code review on task 3-10 found that startSelectedServer()'s .online success path never produced an ActivityEntry, contradicting the spec's literal start/stop/backup activity history requirement. The original justification (avoiding a second subscriber on processService.events) didn't apply: startSelectedServer() never touches that stream, so appending directly from its success path is safe. Shares logic with apply(_:)'s activity logging via a new appendActivity(forServerId:status:) helper instead of duplicating name-resolution/insert/trim code. Deliberately did not add a second 'stop requested' entry to stopSelectedServer() -- the reviewer's finding was specifically about the missing start entry, and a stop's outcome is only certain once apply(_:) observes .offline/.crashed via the event stream, so a second immediate entry would just add noise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
stdoutLines(serverId:) keyed 'anything to read' on runningProcesses liveness alone, so a process reaped by the termination-monitoring task before the caller's first stdoutLines call returned nil even though the OS pipe still held every byte the process wrote. This is exactly the case that matters most in production: a server that crashes immediately after launch. Salvages the stdout Pipe into a short-lived terminatedStdout cache on termination, bounded to one entry per server (cleared on claim or on the next start), so buffered output is still delivered. Fixes a recurring flake in streamLogsPopulatesLinesInOrder under parallel test load; confirmed with 8 consecutive clean full-suite runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Commit 12d7805 fixed the exit-before-first-read race by having handleTermination unconditionally salvage a terminated process's stdout Pipe into terminatedStdout. Code review found this reintroduces a more common race: ServerDetailView re-invokes stdoutLines(serverId:) on every server.status transition via .task(id: server.status), so the ordinary stop/crash-with-console-open case calls stdoutLines twice for the same server -- once live (attaching a reader), again after termination. Unconditional salvage handed the same pipe out a second time, risking two readers splitting bytes unpredictably on an in-flight teardown. Adds RunningProcess.claimed, set the moment stdoutLines hands out a live reader. handleTermination now only salvages into terminatedStdout when the pipe was never claimed live, restoring the pre-12d7805 nil result for the already-streamed-live case while keeping the original never-claimed-before-exit fix intact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior commit's pre-commit SwiftFormat hook expanded a couple of single-line if/closure bodies I had written back into their multi-line form, pushing both ServerProcessService.swift and its test file 2 lines past SwiftLint's 400-line file_length ceiling. No behavior change -- condenses a few doc comments to restore headroom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ic to Swift Task 3-11: faithful, share-nothing Swift port of the Tauri/Rust security.rs gateway (authorize/is_mutating_action, check_rate_limit(_with_state), resolve_safe_path, build_audit_entry) plus a 1:1 port of its 19 Rust unit tests to Swift Testing. Self-contained under Sources/Core/Security/ -- not wired into any view model or command, since this app has no user/role/authentication concept yet; a future task connects it once one exists. RateLimiter is an actor whose single method body is the entire critical section (prune/saturate-check/rate-check/insert, no await inside), matching the swift-concurrency skill's guidance to prefer actor isolation over manual locks for shared mutable state; it mirrors Rust's check_rate_limit vs check_rate_limit_with_state split via an explicit, defaulted now: ContinuousClock.Instant parameter so tests inject deterministic instants instead of racing Task.sleep. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…in resolveSafePath Code review for task 3-11 (security.rs port) found that resolveSafePath rejects a '.' path component in any position, while Rust's Path::components() only produces Component::CurDir for a leading '.' (interior '.' segments are silently normalized away by Rust's own parser and never rejected there). Not a security regression -- rejecting is strictly safer than accepting -- and no ported test exercises interior '.', but it is a real fidelity gap worth disclosing explicitly rather than leaving readers to assume byte-for-byte Rust parity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 3-12's swiftui-pro review found errorMessage was set on load()/startSelectedServer()/stopSelectedServer() failure but never displayed -- a failed start (e.g. missing Java path) had zero user-visible signal beyond the status reverting. Replaces the raw String? with an Identifiable ServerListViewModelError wrapper and presents it from RootView via .alert(_:isPresented:presenting:actions:message:), avoiding the Binding(get:set:)-synthesized-from-optional-String anti-pattern the review flagged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 3-12's swiftui-pro review flagged ActivityDrawerView.swift for declaring two types (ActivityDrawerView and private ActivityRow) in one file, violating this phase's established one-type-per-file convention. Pure file move -- ActivityRow now lives in ActivityRow.swift at default internal visibility (no longer private, since it's used from a sibling file, but not public since ActivityDrawerView remains its sole caller). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 3-B (tasks 3-4 through 3-12) is done: domain model, all screens, process lifecycle, log streaming, floating console panel, activity drawer, and the security.rs port are implemented and tested. Records three items the closing swiftui-pro review (task 3-12) intentionally deferred rather than fixed inline, so Phase 4/5 planning has the context. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughmacOSネイティブアプリ（Swift/SwiftUI）に、サーバードメインモデル、JSON永続化、子プロセス起動/停止・ログストリーミング、一覧/詳細/コンソール/アクティビティUI、およびRust security.rs相当の認可・レート制限・安全パス解決・監査ロジックを新規追加。関連する多数のテストと Changesサーバードメイン・プロセス管理・UI
セキュリティ基盤（認可・レート制限・安全パス・監査）
その他: Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ServerListViewModel
participant ServerProcessService
participant Process
participant ActivityLog
ServerListViewModel->>ServerProcessService: start(server)
ServerProcessService->>Process: launch java process
Process-->>ServerProcessService: terminationHandler / stdout events
ServerProcessService-->>ServerListViewModel: ServerProcessEvent(status)
ServerListViewModel->>ActivityLog: appendActivity(status)
ServerListViewModel->>ServerListViewModel: setStatus(server, status)
sequenceDiagram
participant ServerLogViewModel
participant ServerProcessService
participant LogLineBuffer
participant ServerLogView
ServerLogViewModel->>ServerProcessService: stdoutLines(serverId)
ServerProcessService-->>ServerLogViewModel: AsyncStream<String>
loop 定期フラッシュ
ServerLogViewModel->>ServerLogViewModel: pendingLines蓄積
ServerLogViewModel->>LogLineBuffer: flush() / append
end
ServerLogView->>ServerLogViewModel: read lines
ServerLogView->>ServerLogView: auto-scroll to bottom
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Phase 3-B for the native macOS app (apps/native-macos/) adding the first real server-management screens plus a full Swift port of the Classic app’s security.rs logic/tests, while wiring up process lifecycle + live log streaming + floating console panel + activity drawer.
Changes:
- Added server domain/store + SwiftUI shell (split view, server list/detail, activity drawer, error alert surfacing).
- Implemented Java process lifecycle management (
ServerProcessService) with live stdout streaming + stderr draining and regression tests for known races. - Ported
security.rsauthorization/rate-limit/safe-path/audit logic with one-to-one unit tests.
Reviewed changes
Copilot reviewed 38 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/next-phase-plan.md | Updates phase status matrix and adds Phase 3-B handover notes. |
| apps/native-macos/Tests/CoreTests/ServerTests.swift | JSON round-trip + optional-field decode coverage for Server. |
| apps/native-macos/Tests/CoreTests/ServerTemplateTests.swift | JSON round-trip coverage for ServerTemplate. |
| apps/native-macos/Tests/CoreTests/ServerStoreTests.swift | Persist/load behavior for ServersFile and ServerStore. |
| apps/native-macos/Tests/CoreTests/ServerProcessServiceTests.swift | End-to-end process lifecycle + stdout streaming regression tests. |
| apps/native-macos/Tests/CoreTests/ServerLogViewModelTests.swift | Validates log streaming/batching/trimming behavior in the VM. |
| apps/native-macos/Tests/CoreTests/ServerListViewModelTests.swift | Load/selection/error-surfacing tests for list VM. |
| apps/native-macos/Tests/CoreTests/ServerListViewModelProcessTests.swift | End-to-end VM ↔ process-service integration + activity log tests. |
| apps/native-macos/Tests/CoreTests/SecurityTests.swift | Swift port of security.rs unit tests (spec-fidelity assertions). |
| apps/native-macos/Tests/CoreTests/FloatingConsolePanelTests.swift | Verifies NSPanel configuration + controller visibility state machine. |
| apps/native-macos/Sources/Core/Services/ServerProcessService.swift | Actor-owned child-process management + stdout/stderr piping + events. |
| apps/native-macos/Sources/Core/ServerLogViewModel.swift | Bridges stdout stream into buffered/batched observable log lines. |
| apps/native-macos/Sources/Core/ServerLogView.swift | High-performance log rendering (ScrollView + LazyVStack) + autoscroll. |
| apps/native-macos/Sources/Core/ServerListViewModelError.swift | Identifiable error wrapper for SwiftUI alert presentation. |
| apps/native-macos/Sources/Core/ServerListViewModel.swift | Loads servers, starts/stops processes, subscribes to events, logs activity. |
| apps/native-macos/Sources/Core/ServerListView.swift | Sidebar list for servers + load-on-appear task. |
| apps/native-macos/Sources/Core/ServerDetailView.swift | Detail form + log section + floating console panel toggle. |
| apps/native-macos/Sources/Core/Security/SecurityError.swift | Swift error enum mirroring Rust strings for fidelity. |
| apps/native-macos/Sources/Core/Security/SafePathResolver.swift | Ported safe-path join + traversal/drive-prefix rejection. |
| apps/native-macos/Sources/Core/Security/Role.swift | Ported role model + parsing. |
| apps/native-macos/Sources/Core/Security/RateLimiter.swift | Actor-based per-user rate limiter mirroring Rust behavior. |
| apps/native-macos/Sources/Core/Security/Authorization.swift | Ported authorization + mutating-action classification. |
| apps/native-macos/Sources/Core/Security/AuditEntry.swift | Ported audit entry shape + os.Logger emission. |
| apps/native-macos/Sources/Core/RootView.swift | Main app shell + start/stop toolbar + activity drawer + error alert. |
| apps/native-macos/Sources/Core/MCVectorApp.swift | Introduces the real windowed App entry point. |
| apps/native-macos/Sources/Core/FloatingConsolePanelController.swift | Controls the floating console panel lifecycle without double-streaming. |
| apps/native-macos/Sources/Core/FloatingConsolePanel.swift | NSPanel bridge implementation matching confirmed spike config. |
| apps/native-macos/Sources/Core/FloatingConsoleContentView.swift | Panel UI header + shared log view; glass effect limited to header. |
| apps/native-macos/Sources/Core/Domain/ServerTemplate.swift | Server template domain model mirroring TS contract. |
| apps/native-macos/Sources/Core/Domain/ServerStore.swift | Minimal servers.json persistence + default Application Support path. |
| apps/native-macos/Sources/Core/Domain/ServerStatus.swift | Lifecycle enum mirroring TS union raw values. |
| apps/native-macos/Sources/Core/Domain/ServersFile.swift | Top-level persisted servers.json container shape. |
| apps/native-macos/Sources/Core/Domain/Server.swift | Server domain model mirroring TS MinecraftServer. |
| apps/native-macos/Sources/Core/Domain/AutoBackupScheduleType.swift | Enum for backup schedule type mirroring TS union. |
| apps/native-macos/Sources/Core/ActivityRow.swift | Extracted activity row view used by the drawer list. |
| apps/native-macos/Sources/Core/ActivityEntry.swift | Activity log entry model (session-only). |
| apps/native-macos/Sources/Core/ActivityDrawerView.swift | Inspector-hosted activity drawer view. |
| apps/native-macos/Sources/App/Main.swift | Switches non-spike launches to MCVectorApp.main(). |
| .gitignore | Ignores Instruments .trace captures under native-macos. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+233
to
+235
| private static func decodeLine(_ data: Data) -> String { | ||
| String(bytes: data, encoding: .utf8) ?? "" | ||
| } |
Comment on lines
+131
to
+136
| /// Single actor-wide stream of status-change events, not one per | ||
| /// server -- this task's scope doesn't need per-server granularity, and | ||
| /// it establishes the same AsyncStream-based observation pattern task | ||
| /// 3-8 (log streaming) will also use, per its own task description. | ||
| public let events: AsyncStream<ServerProcessEvent> | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/native-macos/Tests/CoreTests/SecurityTests.swift (1)
1-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win未テストのエラーケースの補完を推奨。
SecurityErrorに定義されている11ケースのうち、以下の4ケースがテストで検証されていません:
.invalidRole—Role.parseに不正な文字列を渡した場合.rateLimiterSaturated—RateLimiterのマップがrateLimitMaxEntriesに達した場合.baseMustBeAbsolute—resolveSafePathのbaseが絶対パスでない場合.auditTimestampCreationFailed—Dateが Unix epoch より前の場合（実質到達不能）Rust 側のテストスイートにこれらに対応するテストが存在しない可能性もありますが、Swift ポート独自のエラーケースとしてカバレッジを補完しておくと、将来のリグレッション防止に有効です。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/native-macos/Tests/CoreTests/SecurityTests.swift` around lines 1 - 288, Add Swift tests to cover the currently unverified SecurityError cases by targeting the corresponding symbols Role.parse, RateLimiter.checkRateLimit, resolveSafePath, and buildAuditEntry. Create assertions for invalid role parsing to hit .invalidRole, fill RateLimiter up to rateLimitMaxEntries to trigger .rateLimiterSaturated, pass a non-absolute base into resolveSafePath to exercise .baseMustBeAbsolute, and add a focused check around audit timestamp creation so .auditTimestampCreationFailed is covered or explicitly documented if unreachable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/native-macos/Sources/Core/Domain/ServerStore.swift`:
- Around line 51-66: The fallback path in ServerStore.defaultFileURL currently
hides Application Support resolution failures by silently using
FileManager.temporaryDirectory, so update this method to surface the situation
clearly. Either add a warning/log when the fallback is chosen or adjust the
documentation comment on ServerStore.defaultFileURL to explicitly state that
temporaryDirectory may be used and data can be volatile. Keep the behavior
consistent with save()/load() expectations and reference
defaultFileURL/appDirectory in the fix.
---
Nitpick comments:
In `@apps/native-macos/Tests/CoreTests/SecurityTests.swift`:
- Around line 1-288: Add Swift tests to cover the currently unverified
SecurityError cases by targeting the corresponding symbols Role.parse,
RateLimiter.checkRateLimit, resolveSafePath, and buildAuditEntry. Create
assertions for invalid role parsing to hit .invalidRole, fill RateLimiter up to
rateLimitMaxEntries to trigger .rateLimiterSaturated, pass a non-absolute base
into resolveSafePath to exercise .baseMustBeAbsolute, and add a focused check
around audit timestamp creation so .auditTimestampCreationFailed is covered or
explicitly documented if unreachable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6256ff43-6c76-4cd7-a9c3-9906859c35b8
⛔ Files ignored due to path filters (3)
spec/spike/nspanel-active.pngis excluded by!**/*.pngspec/spike/nspanel-inactive.pngis excluded by!**/*.pngspec/spike/panel-window-active.pngis excluded by!**/*.png
📒 Files selected for processing (39)
.gitignoreapps/native-macos/Sources/App/Main.swiftapps/native-macos/Sources/Core/ActivityDrawerView.swiftapps/native-macos/Sources/Core/ActivityEntry.swiftapps/native-macos/Sources/Core/ActivityRow.swiftapps/native-macos/Sources/Core/Domain/AutoBackupScheduleType.swiftapps/native-macos/Sources/Core/Domain/Server.swiftapps/native-macos/Sources/Core/Domain/ServerStatus.swiftapps/native-macos/Sources/Core/Domain/ServerStore.swiftapps/native-macos/Sources/Core/Domain/ServerTemplate.swiftapps/native-macos/Sources/Core/Domain/ServersFile.swiftapps/native-macos/Sources/Core/FloatingConsoleContentView.swiftapps/native-macos/Sources/Core/FloatingConsolePanel.swiftapps/native-macos/Sources/Core/FloatingConsolePanelController.swiftapps/native-macos/Sources/Core/MCVectorApp.swiftapps/native-macos/Sources/Core/RootView.swiftapps/native-macos/Sources/Core/Security/AuditEntry.swiftapps/native-macos/Sources/Core/Security/Authorization.swiftapps/native-macos/Sources/Core/Security/RateLimiter.swiftapps/native-macos/Sources/Core/Security/Role.swiftapps/native-macos/Sources/Core/Security/SafePathResolver.swiftapps/native-macos/Sources/Core/Security/SecurityError.swiftapps/native-macos/Sources/Core/ServerDetailView.swiftapps/native-macos/Sources/Core/ServerListView.swiftapps/native-macos/Sources/Core/ServerListViewModel.swiftapps/native-macos/Sources/Core/ServerListViewModelError.swiftapps/native-macos/Sources/Core/ServerLogView.swiftapps/native-macos/Sources/Core/ServerLogViewModel.swiftapps/native-macos/Sources/Core/Services/ServerProcessService.swiftapps/native-macos/Tests/CoreTests/FloatingConsolePanelTests.swiftapps/native-macos/Tests/CoreTests/SecurityTests.swiftapps/native-macos/Tests/CoreTests/ServerListViewModelProcessTests.swiftapps/native-macos/Tests/CoreTests/ServerListViewModelTests.swiftapps/native-macos/Tests/CoreTests/ServerLogViewModelTests.swiftapps/native-macos/Tests/CoreTests/ServerProcessServiceTests.swiftapps/native-macos/Tests/CoreTests/ServerStoreTests.swiftapps/native-macos/Tests/CoreTests/ServerTemplateTests.swiftapps/native-macos/Tests/CoreTests/ServerTests.swiftspec/next-phase-plan.md
Comment on lines
+51
to
+66
| public static func defaultFileURL(fileManager: FileManager = .default) -> URL { | ||
| let supportDirectory = (try? fileManager.url( | ||
| for: .applicationSupportDirectory, | ||
| in: .userDomainMask, | ||
| appropriateFor: nil, | ||
| create: true, | ||
| )) ?? fileManager.temporaryDirectory | ||
|
|
||
| let appDirectory = supportDirectory.appendingPathComponent( | ||
| self.applicationSupportSubdirectoryName, | ||
| isDirectory: true, | ||
| ) | ||
| try? fileManager.createDirectory(at: appDirectory, withIntermediateDirectories: true) | ||
|
|
||
| return appDirectory.appendingPathComponent("servers.json", isDirectory: false) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
defaultFileURL の temporaryDirectory フォールバック時にエラーが表面化しないパスがある。
Application Support ディレクトリの解決に失敗した場合、temporaryDirectory にフォールバックします。このフォールバックが成功すると、save()/load() はエラーを投げずに成功しますが、データは揮発性の一時ディレクトリに書き込まれ、macOS によって削除される可能性があります。ドキュメントコメントの「any real failure still surfaces later as a ServerStore.load()/save() error」という記述は、このパスでは成立しません。
フォールバック時に警告ログを出力するか、ドキュメントコメントを修正してこの挙動を明記することを推奨します。
🛡️ 提案する修正: フォールバック時の警告ログ追加
public static func defaultFileURL(fileManager: FileManager = .default) -> URL {
- let supportDirectory = (try? fileManager.url(
+ let supportDirectory: URL
+ if let resolved = try? fileManager.url(
for: .applicationSupportDirectory,
in: .userDomainMask,
appropriateFor: nil,
create: true,
- )) ?? fileManager.temporaryDirectory
+ ) {
+ supportDirectory = resolved
+ } else {
+ // Application Support の解決に失敗した場合は一時ディレクトリにフォールバックする。
+ // データが揮発性の場所に保存されるため、実環境では極めて稀だが注意が必要。
+ print("Warning: Failed to resolve Application Support directory; falling back to temporary directory.")
+ supportDirectory = fileManager.temporaryDirectory
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/native-macos/Sources/Core/Domain/ServerStore.swift` around lines 51 -
66, The fallback path in ServerStore.defaultFileURL currently hides Application
Support resolution failures by silently using FileManager.temporaryDirectory, so
update this method to surface the situation clearly. Either add a warning/log
when the fallback is chosen or adjust the documentation comment on
ServerStore.defaultFileURL to explicitly state that temporaryDirectory may be
used and data can be volatile. Keep the behavior consistent with save()/load()
expectations and reference defaultFileURL/appDirectory in the fix.
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.
Summary
Phase 3-B implementation of the MC-Vector native macOS app (
apps/native-macos/), coveringspec/phase-tasks.mdtasks 3-4 through 3-12. Builds on Phase 3-A's confirmed spike decisions (NSPanel bridge for floating panels, entitlements-free Java process launch, ScrollView+LazyVStack for high-frequency log rendering).Server/ServerStatus/ServerTemplatedomain model + minimalservers.jsonstoreNavigationSplitView) + the app's first real windowed entry pointServerProcessServiceactor), stdin-based graceful stop with timeout→SIGTERM escalation.inspectorsecurity.rs's authorization/rate-limit/path-safety/audit logic and all 19 of its unit tests (no logic shared between the Tauri and Native apps, per this project's standing rule — port only, not wired into UI since there's no role/auth concept yet)swiftui-proreview; fixed the two findings worth fixing now (a real error-surfacing gap, a file-per-type split), documented the rest as Phase 4/5 handover notes inspec/next-phase-plan.mdReal bugs found and fixed along the way (via code review, not just implementation):
Test plan
swift buildclean, zero warningsswift test— 74/74 tests pass, re-run repeatedly (8-11x in places with prior flakiness) with no failuresswiftformat --lint/swiftlintclean🤖 Generated with Claude Code
https://claude.ai/code/session_01W6ZTictDPbmypEwnXQn8Zg
Summary by CodeRabbit
新機能
バグ修正
テスト