Add multi-app server support - #19
Conversation
matt-greathouse
left a comment
There was a problem hiding this comment.
Code review: multi-app server support
Solid refactor overall. Replacing the single serverPid_/serverUrl_ triple with a map<string, ManagedServer> keyed by config name is the right model, and swapping the blocking WaitForServerHealth spin loop for a state machine driven by wxTimer is a real improvement over master. The new PlanServerPorts is a good extraction — pure, injectable via the portAvailable callback, and genuinely unit-testable, which is why it ended up with the best test coverage in the PR. SafeStateName correctly treats config keys as untrusted filename input (sanitize + FNV-1a suffix), and the tests cover ../ and / cases.
Ten comments inline. Nothing here looks like a data-loss or memory-safety bug; the theme is blocking I/O moved onto the UI thread plus some state-machine edges.
Main concerns
CreatePopupMenunow does heavy blocking I/O on every right-click — two config reads, a directory scan, apopen("ps ax")fork, and multiple 1 s-timeout socket probes. On master this ran only at startup and on explicit Start. This is the finding I'd fix first; it is user-visible on the hottest UI path.- The adoption pass health-checks servers it already manages, because
validis computed before thepidAlreadyManaged/ status short-circuits. Hoisting those checks removes most of #1's cost. HttpHealthOkinside a 100 ms timer tick can overrun its own interval during startup.wxMessageBoxfrom the timer handler re-entersPollServersand stacks one modal per failing server.IsProcessAliveiskill(pid, 0), which is true for un-reaped zombie children — the "exited before becoming healthy" fast path won't reliably fire for tray-spawned servers. This file's ownWaitForProcessExit(pid, reapChild)already handles this distinction.
Smaller items: occupiedPorts is built two different ways in OnStartServer vs ToggleServer (and excludes Stopping in one of them); the post-SIGKILL-failure branch marks a server Running (🟢) right after telling the user it wouldn't stop; the "no available port" message hides the 100-port scan window; ListTrayAppServerStatePaths discards partial results on error.
Testing: the CLI flag-validation and PlanServerPorts tests are welcome, and the TrayServerState additions pin down the path-traversal behavior. The basePort fallback and the start + 100 boundary are the two untested branches worth adding. The tray state machine itself is untested, which is understandable for GUI code — but PollServers' transitions are the most intricate logic in the PR, and much of it would be testable if the liveness/health probes were injected the way portAvailable already is. Worth considering as follow-up rather than a blocker.
Note that the PR description says tests were not run; please confirm the suite passes before merge.
Generated by Claude Code
Summary
Testing