feat: add native Windows desktop app (C#/WPF/.NET 8)#142
Open
2witstudios wants to merge 2 commits intomainfrom
Open
feat: add native Windows desktop app (C#/WPF/.NET 8)#1422witstudios wants to merge 2 commits intomainfrom
2witstudios wants to merge 2 commits intomainfrom
Conversation
Complete WPF desktop application for PPG with MVVM architecture: - Models: Manifest types matching ppg-cli TypeScript definitions, API request/response records - Services: REST API client, reconnecting WebSocket service, DPAPI token storage, JSON settings - ViewModels: Main, Sidebar (tree hierarchy), Terminal, HomeDashboard (stats + heatmap), Spawn, Settings - Views: Sidebar tree, WebView2 terminal (xterm.js), dashboard with stats cards + commit heatmap, spawn dialog, settings panel - Terminal: WebView2 + xterm.js bridge with bidirectional message passing - Converters: Status-to-color, bool-to-visibility, inverse-bool for data binding - DI container via Microsoft.Extensions.DependencyInjection - CommunityToolkit.Mvvm source generators for ObservableProperty/RelayCommand
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR implements a comprehensive, multi-platform version of the ppg tool featuring: an iOS mobile app (SwiftUI) with REST and WebSocket clients; a Windows desktop app (WPF); an enhanced Node.js/Fastify server with TLS, authentication, and WebSocket support; and refactored CLI commands that delegate to centralized core operations for kill, merge, restart, and spawn workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
ios/PPGMobile/PPGMobile/Assets.xcassets/AppIcon.appiconset/Contents.json-2-8 (1)
2-8:⚠️ Potential issue | 🟡 MinorApp icon image file not referenced.
The
imagesentry specifies the 1024x1024 icon slot but lacks a"filename"property pointing to an actual image file. The app will build and run, but will display a blank/default app icon.If this is intentional (placeholder for later), consider adding a TODO. Otherwise, add the icon image to this directory and update the entry:
Example with filename reference
"images": [ { "idiom": "universal", + "filename": "AppIcon.png", "platform": "ios", "size": "1024x1024" } ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Assets.xcassets/AppIcon.appiconset/Contents.json` around lines 2 - 8, The images entry for the 1024x1024 app icon is missing a "filename" property, so the icon slot is not pointing to an actual image; fix by adding the icon image file into the app icon asset folder and updating the images array entry for the "1024x1024" size to include "filename": "<your-icon-filename.png>" (or, if this is intentionally a placeholder, add a clear TODO/comment in the asset JSON indicating the missing file and why it's omitted).windows/PPGDesktop/Models/AppSettings.cs-10-11 (1)
10-11:⚠️ Potential issue | 🟡 MinorConsider adding
[JsonIgnore]to prevent accidental token serialization.The
Tokenproperty is populated from DPAPI secure storage (perSettingsService.Load()), but having[JsonPropertyName("token")]means it could be serialized to the JSON settings file ifSave()is called whileTokenis set. If the intent is to keep the token exclusively in DPAPI-protected storage, consider:🛡️ Proposed fix
- [JsonPropertyName("token")] + [JsonIgnore] public string? Token { get; set; }If the token should be persisted to JSON as a fallback, the current implementation is fine, but verify this aligns with the security model described in the PR (DPAPI-encrypted bearer token storage).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@windows/PPGDesktop/Models/AppSettings.cs` around lines 10 - 11, The Token property on AppSettings (public string? Token) is marked with JsonPropertyName and can be written to settings JSON; to prevent accidental serialization move token storage exclusively to DPAPI by adding the [JsonIgnore] attribute to the Token property in AppSettings, and ensure SettingsService.Load()/Save() continues to read/write token from DPAPI only; if you intentionally want a JSON fallback, document that behavior in AppSettings and SettingsService instead of leaving JsonPropertyName alone.src/server/routes/spawn.test.ts-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
afterEachteardown to close the Fastify app and prevent resource leaks.The app is created in
beforeEach(line 55–58) but never closed. This mirrors the pattern inconfig.test.ts(line 28–31), which correctly includesafterEach(async () => { await app?.close(); })to release handles.Proposed fix
-import { describe, test, expect, vi, beforeEach } from 'vitest'; +import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest';Then add after the
beforeEachblock:beforeEach(async () => { vi.clearAllMocks(); app = await buildApp(); }); + + afterEach(async () => { + await app.close(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/spawn.test.ts` at line 1, The tests create a Fastify app in the beforeEach block but never close it, causing resource leaks; add an afterEach teardown that awaits closing the app (use the same app variable created in beforeEach) by adding an afterEach(async () => { await app?.close(); }) to mirror config.test.ts and ensure the Fastify instance is properly closed between tests.src/core/spawn.ts-127-129 (1)
127-129:⚠️ Potential issue | 🟡 MinorValidate
countbefore creating the worktree.
count <= 0currently still creates the worktree/tmux resources and returns with no agents. Reject invalid values up front.Based on learnings, use `PpgError` hierarchy with typed error codes including `INVALID_ARGS`.🧪 Guard invalid spawn counts
const count = opts.count ?? 1; + if (!Number.isInteger(count) || count < 1) { + throw new PpgError('count must be an integer >= 1', 'INVALID_ARGS'); + } const userVars = opts.userVars ?? {};Also applies to: 171-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/spawn.ts` around lines 127 - 129, Validate the spawn `count` immediately after reading `opts.count` (the `count` variable) and before calling `requireManifest` or creating any worktree/tmux resources: if count is not a positive integer (<= 0 or NaN), throw a PpgError with error code INVALID_ARGS and a clear message (e.g., "invalid count: must be >= 1"). Ensure the same guard is applied in the other creation path referenced around the block for lines 171-179 (the agent/worktree creation logic) so no resources are allocated when count is invalid.src/server/auth.ts-261-269 (1)
261-269:⚠️ Potential issue | 🟡 MinorAuthorization scheme parsing should be case-insensitive.
HTTP auth scheme tokens are case-insensitive; strict
startsWith('Bearer ')can reject valid clients.🔧 Robust Authorization parsing
- if (typeof authHeader !== 'string' || !authHeader.startsWith('Bearer ')) { + if (typeof authHeader !== 'string' || !/^Bearer\s+/i.test(authHeader)) { rateLimiter.record(ip); reply.code(401).send({ error: 'Missing or malformed Authorization header' }); return; } - const token = authHeader.slice(7).trim(); + const token = authHeader.replace(/^Bearer\s+/i, '').trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/auth.ts` around lines 261 - 269, The Authorization header check in auth.ts uses a case-sensitive startsWith('Bearer ') which can reject valid headers; update the parsing around authHeader (and the subsequent token extraction into token / TokenEntry logic) to perform a case-insensitive scheme check (for example compare the scheme lowercased or use a case-insensitive regex) and then extract the token string safely (trim and validate non-empty) before proceeding to rateLimiter.record(ip) and reply.code(401). Ensure you still handle missing/malformed headers the same way but accept variants like "bearer" or "BEARER".src/core/serve.test.ts-47-56 (1)
47-56:⚠️ Potential issue | 🟡 MinorUse platform-independent path expectations in assertions.
These assertions hardcode POSIX separators, which can fail on Windows.
✅ Cross-platform expectation fix
- expect(fs.unlink).toHaveBeenCalledWith('/fake/project/.ppg/serve.pid'); + expect(fs.unlink).toHaveBeenCalledWith(path.join('/fake/project', '.ppg', 'serve.pid')); ... - expect(fs.unlink).toHaveBeenCalledWith('/fake/project/.ppg/serve.pid'); + expect(fs.unlink).toHaveBeenCalledWith(path.join('/fake/project', '.ppg', 'serve.pid'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/serve.test.ts` around lines 47 - 56, The test assertions use a hardcoded POSIX path string; update them to build the expected PID filepath platform-independently (e.g., use path.join or path.resolve) when calling getServePid and in the expect for fs.unlink so the tests work on Windows as well—locate the tests around getServePid in src/core/serve.test.ts and replace literal '/fake/project/.ppg/serve.pid' with a path.join-based expression that constructs the same segments (project root, '.ppg', 'serve.pid').src/server/routes/status.ts-16-21 (1)
16-21:⚠️ Potential issue | 🟡 MinorTiming side-channel on token length.
The early return when buffer lengths differ allows an attacker to determine the token length via timing analysis. While the actual comparison uses
crypto.timingSafeEqual, the length check leaks information.🛡️ Proposed fix to mitigate length oracle
function timingSafeEqual(a: string, b: string): boolean { const aBuffer = Buffer.from(a); const bBuffer = Buffer.from(b); - if (aBuffer.length !== bBuffer.length) return false; - return crypto.timingSafeEqual(aBuffer, bBuffer); + // Pad to same length to prevent length oracle, then compare + const maxLen = Math.max(aBuffer.length, bBuffer.length); + const aPadded = Buffer.alloc(maxLen); + const bPadded = Buffer.alloc(maxLen); + aBuffer.copy(aPadded); + bBuffer.copy(bPadded); + // Both comparisons must pass + return crypto.timingSafeEqual(aPadded, bPadded) && aBuffer.length === bBuffer.length; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/status.ts` around lines 16 - 21, The timingSafeEqual function leaks token length because it returns early when buffer lengths differ; change it to produce fixed-size, equal-length inputs before calling crypto.timingSafeEqual. For example, instead of comparing raw buffers and early-returning, normalize both inputs to a constant-length representation (e.g., compute a fixed-length digest such as SHA-256 for each string using crypto.createHash('sha256').update(...).digest()) and then call crypto.timingSafeEqual on those digests so comparison time does not reveal the original lengths; update the timingSafeEqual function to implement this change.windows/PPGDesktop/MainWindow.xaml.cs-18-23 (1)
18-23:⚠️ Potential issue | 🟡 MinorFire-and-forget async call may swallow exceptions.
The discard
_ = terminalVm.AttachAsync(...)will silently swallow any exceptions thrown byAttachAsync. Consider awaiting or adding error handling to surface failures to the user.💡 Suggested fix with error handling
viewModel.Sidebar.AgentSelected += agent => { var terminalVm = App.Services.GetRequiredService<TerminalViewModel>(); viewModel.CurrentView = terminalVm; - _ = terminalVm.AttachAsync(agent.Id, agent.DisplayName); + _ = terminalVm.AttachAsync(agent.Id, agent.DisplayName) + .ContinueWith(t => + { + if (t.IsFaulted) + { + // Log or surface error to user + System.Diagnostics.Debug.WriteLine($"AttachAsync failed: {t.Exception?.InnerException?.Message}"); + } + }, System.Threading.Tasks.TaskScheduler.FromCurrentSynchronizationContext()); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@windows/PPGDesktop/MainWindow.xaml.cs` around lines 18 - 23, The event handler for viewModel.Sidebar.AgentSelected is fire-and-forget and discards exceptions from TerminalViewModel.AttachAsync; change the lambda to an async handler and await AttachAsync inside a try/catch so errors are not swallowed. Specifically, update the Sidebar.AgentSelected handler to set viewModel.CurrentView to the TerminalViewModel, then await terminalVm.AttachAsync(agent.Id, agent.DisplayName) inside a try block and handle failures in the catch by logging and/or surfacing an error to the user (e.g., via the view model or a user-visible notification).ios/PPGMobile/PPGMobile/Views/Spawn/SpawnView.swift-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorNavigation to spawned worktree is not wired up.
spawnedWorktreeIdis set on line 169 after a successful spawn, andnavigationDestination(for: String.self)is defined on lines 62-64, but there's no mechanism to trigger the navigation. The state variable is set but never consumed.To navigate programmatically in SwiftUI, you'd typically use a
NavigationPathor a binding that triggers the destination.🔧 Possible fix using NavigationPath
struct SpawnView: View { `@Environment`(AppState.self) private var appState + `@State` private var navigationPath = NavigationPath() // ... var body: some View { - NavigationStack { + NavigationStack(path: $navigationPath) { Form { // ... } // ... } } `@MainActor` private func spawnWorktree() async { // ... do { let response = try await appState.client.spawn(...) await appState.manifestStore.refresh() clearForm() - spawnedWorktreeId = response.worktreeId + navigationPath.append(response.worktreeId) } catch { // ... } // ... }Also applies to: 62-64, 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Views/Spawn/SpawnView.swift` at line 16, spawnedWorktreeId is assigned after a successful spawn but never drives navigation; wire it into a NavigationStack by adding a NavigationPath (or a `@State` var path: NavigationPath) in SpawnView and, inside the success branch where spawnedWorktreeId is set, push the id onto the path (e.g., path.append(spawnedWorktreeId!)) so navigationDestination(for: String.self) is triggered; update the view to use NavigationStack(path: $path) instead of a simple NavigationView and remove/stop relying on the unused spawnedWorktreeId alone.ios/PPGMobile/PPGMobileTests/ServerConnectionTests.swift-110-115 (1)
110-115:⚠️ Potential issue | 🟡 MinorIncorrect test expectation: missing port should default to 7700, not return nil.
Based on the
fromQRCodeimplementation in the relevant code snippet:let port = params["port"].flatMap(Int.init) ?? 7700A missing
portparameter defaults to 7700, so the test on line 113 (ppg://connect?host=x&token=t) should succeed and return a connection with port 7700, not returnnil.🐛 Proposed fix
`@Test`("fromQRCode returns nil when required fields are missing") func rejectsMissingFields() { `#expect`(ServerConnection.fromQRCode("ppg://connect?host=x&port=1") == nil) // no token - `#expect`(ServerConnection.fromQRCode("ppg://connect?host=x&token=t") == nil) // no port `#expect`(ServerConnection.fromQRCode("ppg://connect?port=1&token=t") == nil) // no host } + + `@Test`("fromQRCode defaults port to 7700 when missing") + func defaultsPort() { + let parsed = ServerConnection.fromQRCode("ppg://connect?host=x&token=t") + `#expect`(parsed?.port == 7700) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobileTests/ServerConnectionTests.swift` around lines 110 - 115, The test incorrectly expects ServerConnection.fromQRCode to return nil when the port parameter is missing; update the test in ServerConnectionTests.swift to reflect that fromQRCode (in ServerConnection.fromQRCode) defaults port to 7700: change the assertion for "ppg://connect?host=x&token=t" to expect a non-nil ServerConnection and verify its port equals 7700 (leave the other two expectations for missing token and missing host as nil).src/core/operations/merge.test.ts-327-328 (1)
327-328:⚠️ Potential issue | 🟡 MinorTimestamp validity assertion is ineffective.
Line 328 uses
new Date(...)withnot.toThrow(), but invalid dates don’t throw in JS. This can hide badmergedAtvalues.✅ Tighten the assertion
- // Should be a valid ISO date - expect(() => new Date(latestManifest.worktrees['wt-abc123'].mergedAt!)).not.toThrow(); + // Should be a valid ISO date + expect(Number.isNaN(Date.parse(latestManifest.worktrees['wt-abc123'].mergedAt!))).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/operations/merge.test.ts` around lines 327 - 328, The current test uses expect(() => new Date(latestManifest.worktrees['wt-abc123'].mergedAt!)).not.toThrow(), which is ineffective because Date construction never throws; instead assert the timestamp is a valid ISO/date by checking the parsed time is not NaN — e.g. replace the throw-based check with an assertion that Date.parse(latestManifest.worktrees['wt-abc123'].mergedAt!) (or new Date(...).getTime()) is not NaN/finite so the test actually fails for invalid timestamps.
🧹 Nitpick comments (27)
src/lib/paths.ts (1)
114-128: Avoid drift betweenserveJsonPathandserveStatePath.Both helpers currently build the same literal path. Consider delegating one to the other so future path layout changes can’t diverge.
♻️ Optional deduplication
export function serveStatePath(projectRoot: string): string { - return path.join(ppgDir(projectRoot), 'serve.json'); + return serveJsonPath(projectRoot); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/paths.ts` around lines 114 - 128, The two helpers duplicate the same path logic; update serveStatePath to delegate to serveJsonPath (or vice versa) so they share a single implementation and cannot drift—replace the body of serveStatePath with a call to serveJsonPath(projectRoot) (retaining ppgDir usage via the existing serveJsonPath) so future changes to the serve.json location only need to be made in one place.src/core/serve.test.ts (1)
5-7: Usevi.importActualfor partial mocks to match repo convention.Switching to
vi.importActualkeeps this aligned with the project’s Vitest mocking pattern.As per coding guidelines, use `vi.mock` with `vi.importActual` for partial mocks in tests.♻️ Mock factory adjustment
-vi.mock('../lib/paths.js', async (importOriginal) => { - const actual = await importOriginal() as Record<string, unknown>; +vi.mock('../lib/paths.js', async () => { + const actual = await vi.importActual<Record<string, unknown>>('../lib/paths.js'); return { ...actual,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/serve.test.ts` around lines 5 - 7, The test's partial mock uses the mock factory param importOriginal; replace that with the repo-conventional vi.importActual call inside the vi.mock factory: in the vi.mock('../lib/paths.js', async (importOriginal) => { ... }) block, stop using the importOriginal parameter and instead call vi.importActual to obtain the actual module (assign to the existing actual variable) before returning the partial mock; keep the rest of the factory return unchanged so the partial mock behavior remains the same.src/lib/paths.test.ts (1)
90-146: Align new test names with the repo’sgiven X, should Yconvention.The newly added test cases use short names; please rename them to the required behavior-driven format for consistency.
As per coding guidelines, use
describe('unitName')→test('given X, should Y')pattern for test organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/paths.test.ts` around lines 90 - 146, Rename the flat short-named tests to follow the repo convention by grouping related assertions under describe blocks (e.g., describe('paths') or describe('tls paths') as appropriate) and changing each test invocation to the behavior-driven style test('given <input>, should <expected outcome>') — for example replace test('serveDir', ...) with test('given ROOT, should return .ppg/serve path') and similarly rename tests for tlsDir, tlsCaKeyPath, tlsCaCertPath, tlsServerKeyPath, tlsServerCertPath, worktreeBaseDir, worktreePath, globalPpgDir, globalPromptsDir, globalTemplatesDir, globalSwarmsDir, serveStatePath, and servePidPath while keeping the same assertions and function calls (serveDir, tlsDir, tlsCaKeyPath, tlsCaCertPath, tlsServerKeyPath, tlsServerCertPath, worktreeBaseDir, worktreePath, globalPpgDir, globalPromptsDir, globalTemplatesDir, globalSwarmsDir, serveStatePath, servePidPath).src/server/routes/status.ts (1)
49-71: Inconsistent error handling with centralizederror-handler.ts.This route defines its own
ppgErrorToStatusmapping and error handler that diverges from the centralizederror-handler.ts:
NOT_INITIALIZED: 503 here vs 409 in error-handler.tsMANIFEST_LOCK: 503 here vs 409 in error-handler.ts- Response shape differs:
{ error, code }vs{ error: { code, message } }Consider using
registerErrorHandler(fastify)fromerror-handler.tsfor consistency, or document why status routes intentionally use different semantics.♻️ Suggested refactor to use centralized error handler
+import { registerErrorHandler } from '../error-handler.js'; import { NotInitializedError, PpgError } from '../../lib/errors.js'; -const ppgErrorToStatus: Record<string, number> = { - NOT_INITIALIZED: 503, - MANIFEST_LOCK: 503, - WORKTREE_NOT_FOUND: 404, - AGENT_NOT_FOUND: 404, -}; export default async function statusRoutes( fastify: FastifyInstance, options: StatusRouteOptions, ): Promise<void> { const { projectRoot, bearerToken } = options; fastify.addHook('onRequest', authenticate(bearerToken)); - fastify.setErrorHandler((error, _request, reply) => { - if (error instanceof PpgError) { - const status = ppgErrorToStatus[error.code] ?? 500; - reply.code(status).send({ error: error.message, code: error.code }); - return; - } - reply.code(500).send({ error: 'Internal server error' }); - }); + registerErrorHandler(fastify);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/status.ts` around lines 49 - 71, The route-level error handling in statusRoutes duplicates ppgErrorToStatus and returns a different response shape than the centralized handler; replace the custom handler by registering the shared handler from error-handler.ts: call registerErrorHandler(fastify) inside statusRoutes (and remove the ppgErrorToStatus mapping and setErrorHandler block) so PpgError codes and response shape ({ error: { code, message } }) are consistent across the app; if there is an intentional divergence, document the reason in a comment above statusRoutes and ensure the mapping and response shape exactly match the centralized registerErrorHandler implementation.ios/PPGMobile/PPGMobile/Models/DashboardModels.swift (1)
91-107: Consider cachingISO8601DateFormatterto avoid repeated allocations.Creating a new
ISO8601DateFormatter()instance on every property access is inefficient, especially when rendering lists of agents or worktrees. The formatter is thread-safe and can be shared.♻️ Suggested refactor to cache the date formatter
+// MARK: - Shared Date Formatter + +private let iso8601Formatter: ISO8601DateFormatter = { + let formatter = ISO8601DateFormatter() + return formatter +}() + // MARK: - AgentEntry UI Extensions extension AgentEntry { var startDate: Date? { - ISO8601DateFormatter().date(from: startedAt) + iso8601Formatter.date(from: startedAt) } } // MARK: - WorktreeEntry UI Extensions extension WorktreeEntry { var createdDate: Date? { - ISO8601DateFormatter().date(from: createdAt) + iso8601Formatter.date(from: createdAt) } var mergedDate: Date? { - mergedAt.flatMap { ISO8601DateFormatter().date(from: $0) } + mergedAt.flatMap { iso8601Formatter.date(from: $0) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Models/DashboardModels.swift` around lines 91 - 107, Cache a shared ISO8601DateFormatter instead of allocating one on every property access: add a single shared formatter (e.g., a static let iso8601Formatter = ISO8601DateFormatter() somewhere accessible) and update AgentEntry.startDate, WorktreeEntry.createdDate, and WorktreeEntry.mergedDate to use that shared formatter rather than creating new ISO8601DateFormatter() instances each call; ensure the formatter is declared in a thread-safe static context (for example as a static property on an extension or a small DateFormatter helper type) so all three computed properties reference it.src/commands/serve.ts (2)
66-95: Redundant manifest read.
requireManifest(line 68) already reads and returns the manifest, but it's called without using the return value. ThenreadManifestis called again at line 93 to get thesessionName. This results in reading the same file twice.♻️ Proposed fix to eliminate redundant read
export async function serveStartCommand(options: ServeStartOptions): Promise<void> { const projectRoot = await getRepoRoot(); - await requireManifest(projectRoot); + const manifest = await requireManifest(projectRoot); const port = options.port!; const host = options.host!; ... // Start daemon in a tmux window - const manifest = await readManifest(projectRoot); const sessionName = manifest.sessionName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/serve.ts` around lines 66 - 95, In serveStartCommand, avoid the double manifest read: change the call to requireManifest(projectRoot) so you capture and reuse its returned manifest (e.g., const manifest = await requireManifest(projectRoot)) and then remove the subsequent readManifest(projectRoot) call; use manifest.sessionName where sessionName is needed (keep getRepoRoot, tmux.ensureSession, and other logic unchanged).
6-6: Unused import:runServeDaemonis imported but never used.The
runServeDaemonfunction is imported butserveDaemonCommand(line 197-200) callsstartServerdirectly instead. This import appears to be dead code.🧹 Proposed fix to remove unused import
-import { runServeDaemon, isServeRunning, getServePid, getServeInfo, readServeLog } from '../core/serve.js'; +import { isServeRunning, getServePid, getServeInfo, readServeLog } from '../core/serve.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/serve.ts` at line 6, Remove the dead import runServeDaemon from the import list in serve.ts since serveDaemonCommand calls startServer directly; update the import statement that currently includes runServeDaemon (from '../core/serve.js') to only import the actually used symbols (isServeRunning, getServePid, getServeInfo, readServeLog) and run a quick lint/TS check to ensure no other references to runServeDaemon remain.PPG CLI/PPG CLI/WebSocketManager.swift (1)
109-118: Potential race condition indeinit.The
deinitaccesses and modifies instance properties (intentionalDisconnect,pingTimer,task,session) without synchronization on thequeue. Whiledeinitis called when there's only one reference holder, there could still be in-flight async operations on the queue that reference these properties.Consider wrapping the cleanup in a synchronous queue operation, or ensure all async work is cancelled first.
♻️ Safer deinit with queue synchronization
deinit { - // Synchronous cleanup — safe because we're the last reference holder. - intentionalDisconnect = true - pingTimer?.cancel() - pingTimer = nil - task?.cancel(with: .goingAway, reason: nil) - task = nil - session?.invalidateAndCancel() - session = nil + // Synchronous cleanup on queue to avoid races with in-flight operations. + queue.sync { + intentionalDisconnect = true + pingTimer?.cancel() + pingTimer = nil + task?.cancel(with: .goingAway, reason: nil) + task = nil + session?.invalidateAndCancel() + session = nil + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PPG` CLI/PPG CLI/WebSocketManager.swift around lines 109 - 118, deinit is mutating instance state off the protected queue which can race with in-flight async operations; wrap the entire cleanup in a synchronous call to the object's serial queue (e.g., perform a queue.sync { ... } block) and move the mutations there so intentionalDisconnect, pingTimer, task and session are all read/modified on the same queue; inside that sync block first set intentionalDisconnect = true, cancel and nil the pingTimer, cancel the task with .goingAway and nil it, then invalidateAndCancel the session and nil it, ensuring all async work is cancelled or observes intentionalDisconnect before returning.src/commands/spawn.test.ts (1)
20-31: Consider using the sharedmakeAgentfixture fromsrc/test-fixtures.ts.A
makeAgenthelper already exists insrc/test-fixtures.tswith the same structure. Using it would reduce duplication and keep test fixtures consistent.♻️ Proposed refactor
-import type { AgentEntry } from '../types/manifest.js'; +import { makeAgent } from '../test-fixtures.js'; // ... (remove local makeAgent function) -function makeAgent(id: string, target: string): AgentEntry { - return { - id, - name: 'claude', - agentType: 'claude', - status: 'running', - tmuxTarget: target, - prompt: 'Do work', - startedAt: '2026-02-27T00:00:00.000Z', - sessionId: 'session-1', - }; -} function makeResult(overrides?: Partial<SpawnResult>): SpawnResult { return { worktree: { /* ... */ }, - agents: [makeAgent('ag-1', 'ppg-test:1')], + agents: [makeAgent({ id: 'ag-1', tmuxTarget: 'ppg-test:1', sessionId: 'session-1' })], ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spawn.test.ts` around lines 20 - 31, Replace the local makeAgent helper in spawn.test.ts with the shared fixture: remove the local function named makeAgent and import the exported makeAgent fixture from the test-fixtures module, then use that shared makeAgent wherever the local helper was used; ensure the test still supplies the same id and target args to makeAgent and that any required types (e.g., AgentEntry) remain satisfied by the shared fixture.ios/PPGMobile/PPGMobile/Views/Spawn/SpawnView.swift (1)
34-43: Hardcoded "main" branch may not suit all repositories.Line 36 always inserts "main" as an available base branch. Repositories using "master" or other default branch names won't have their actual default branch listed unless it appears in existing worktrees.
Consider fetching the repository's default branch from the server or manifest instead of hardcoding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Views/Spawn/SpawnView.swift` around lines 34 - 43, The availableBranches computed property currently inserts a hardcoded "main"; change it to use the repository's real default branch from the manifest or app state instead: in availableBranches (and where you reference appState.manifestStore.manifest and manifest.worktrees / wt.baseBranch) look up manifest.defaultBranch (or appState.repository.defaultBranch) and insert that value if present, falling back to "main" only if no default branch is available; if the manifest does not include a default branch, call the existing server/manifest API to obtain the repo default and use that value instead of the literal "main".src/core/operations/restart.test.ts (1)
86-96: Consider using sharedmakeManifestfrom test-fixtures.This local
makeManifesthelper duplicates the one already exported fromsrc/test-fixtures.ts. Using the shared fixture would reduce duplication and ensure consistency across tests.♻️ Suggested change
-import { makeAgent, makeWorktree } from '../../test-fixtures.js'; +import { makeAgent, makeWorktree, makeManifest } from '../../test-fixtures.js'; import type { Manifest } from '../../types/manifest.js'; import type { AgentStatus } from '../../types/manifest.js'; // ... mocks ... -const PROJECT_ROOT = '/tmp/project'; - -function makeManifest(overrides?: Partial<Manifest>): Manifest { - return { - version: 1, - projectRoot: PROJECT_ROOT, - sessionName: 'ppg', - worktrees: {}, - createdAt: '2026-01-01T00:00:00.000Z', - updatedAt: '2026-01-01T00:00:00.000Z', - ...overrides, - }; -} +const PROJECT_ROOT = '/tmp/project';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/operations/restart.test.ts` around lines 86 - 96, The local makeManifest function in restart.test.ts duplicates the shared helper; remove the local makeManifest and import the shared makeManifest exported from src/test-fixtures.ts, then update any references in the file to use that imported helper (ensuring the import brings in the correct Manifest type or adjust the call sites if needed) so tests reuse the canonical fixture and avoid duplication.src/core/agent.ts (1)
247-327: RefactorperformRestartto userestartAgentto eliminate code duplication.
performRestartinsrc/core/operations/restart.tsduplicates the core agent restart logic thatrestartAgentinsrc/core/agent.tsnow provides. Both kill the old agent, create a tmux window, render the prompt template, spawn the new agent, and update the manifest identically.Refactor
performRestartto prepare its parameters (projectRoot, worktree, oldAgent, sessionName, agentConfig, promptText) and delegate torestartAgentfor the core restart operation. This preserves the separation of concerns—orchestration and parameter loading in the operations layer, core agent primitives in the agent layer—while eliminating duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/agent.ts` around lines 247 - 327, The performRestart function duplicates the restart logic now centralized in restartAgent; change performRestart (in src/core/operations/restart.ts) so it gathers the same parameters (projectRoot, worktree, oldAgent, sessionName, agentConfig, promptText) and calls restartAgent({ projectRoot, worktree, oldAgent, sessionName, agentConfig, promptText }) to perform the kill/create-window/render/spawn/update-manifest flow, then return or adapt restartAgent's RestartAgentResult as performRestart's result—remove the duplicated logic and any local calls to killAgent, tmux.createWindow, renderTemplate, spawnAgent, and updateManifest in performRestart so restartAgent owns the core behavior.windows/PPGDesktop/App.xaml.cs (1)
47-59: Unobserved exceptions from fire-and-forget async startup.
StartServicesAsync()is called with discard (_ =), meaning any exceptions will be unobserved. IfWebSocketService.ConnectAsyncthrows (e.g., network unreachable, invalid URL), the exception may silently terminate or cause unpredictable behavior depending on .NET's unobserved task exception settings.Consider wrapping the call or handling exceptions:
♻️ Proposed fix to handle startup exceptions
- // Start WebSocket connection - _ = StartServicesAsync(); + // Start WebSocket connection (fire-and-forget with error handling) + _ = StartServicesAsync().ContinueWith(t => + { + if (t.IsFaulted) + { + // Log or handle startup failure gracefully + System.Diagnostics.Debug.WriteLine($"WebSocket startup failed: {t.Exception?.InnerException?.Message}"); + } + }, TaskScheduler.Default);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@windows/PPGDesktop/App.xaml.cs` around lines 47 - 59, StartServicesAsync is invoked fire-and-forget using "_ = StartServicesAsync();" so exceptions from WebSocketService.ConnectAsync could go unobserved; modify this by handling exceptions either at the call site or inside StartServicesAsync: wrap the await ws.ConnectAsync(settings.ServerUrl, settings.Token) in a try/catch and log the exception (use your app logger/Services.GetRequiredService<...Logger>() or similar), or capture the Task returned by StartServicesAsync and attach a continuation (Task.ContinueWith) to log any faults; ensure StartServicesAsync or its caller does not let exceptions escape unobserved.ios/PPGMobile/PPGMobile/Views/Settings/SettingsView.swift (1)
10-11:showQRErroralert path appears unreachable from this view.Line 10 defines the state and Lines 52-56 render the alert, but nothing in this file sets
showQRError = true. Either wire an invalid-scan callback fromQRScannerViewor remove the dead state/alert path.Also applies to: 52-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Views/Settings/SettingsView.swift` around lines 10 - 11, The SettingsView declares `@State` private var showQRError and renders an alert for it but never sets it, making the alert unreachable; either remove the unused state/alert code or wire an invalid-scan callback from the QRScannerView to toggle it. To fix, locate SettingsView and either (A) remove showQRError and the .alert(...) block, or (B) add a parameter/closure on QRScannerView (e.g., onInvalidScan: () -> Void) and pass a closure that sets showQRError = true when an invalid QR is detected so the alert path becomes reachable.src/server/routes/worktrees.test.ts (1)
69-75: Close Fastify instances after each test to prevent open-handle buildup.Line 69 creates a fresh app for each test, but the suite never closes them. This can leave hanging handles and make test runs flaky.
♻️ Proposed cleanup pattern
-import { describe, test, expect, vi, beforeEach } from 'vitest'; +import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest'; ... const PROJECT_ROOT = '/tmp/project'; +const createdApps: FastifyInstance[] = []; async function buildApp(): Promise<FastifyInstance> { const app = Fastify(); + createdApps.push(app); app.decorate('projectRoot', PROJECT_ROOT); await app.register(worktreeRoutes, { prefix: '/api' }); await app.ready(); return app; } describe('worktreeRoutes', () => { beforeEach(() => { vi.clearAllMocks(); mockManifest.worktrees = {}; }); + + afterEach(async () => { + await Promise.all(createdApps.splice(0).map((app) => app.close())); + });Also applies to: 77-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/worktrees.test.ts` around lines 69 - 75, Tests create Fastify instances via buildApp (which returns a FastifyInstance) but never close them, causing open-handle buildup; update the test suite to ensure each created app is closed after use by calling app.close() (or await app.close()) in an afterEach or finally block where buildApp() is used (references: buildApp, FastifyInstance, app.ready, app.close) so every test that awaits buildApp() cleans up the Fastify instance to prevent flaky tests.src/commands/kill.ts (1)
23-23: Consider extracting the inline type import for clarity.The inline type import works but is unusual. For better readability, consider importing
PaneInfoat the top of the file.♻️ Optional refactor
import { performKill, type KillResult } from '../core/operations/kill.js'; import { getCurrentPaneId } from '../core/self.js'; import { readManifest } from '../core/manifest.js'; import { getRepoRoot } from '../core/worktree.js'; -import { listSessionPanes } from '../core/tmux.js'; +import { listSessionPanes, type PaneInfo } from '../core/tmux.js'; import { output, success, info, warn } from '../lib/output.js';Then on line 23:
- let paneMap: Map<string, import('../core/tmux.js').PaneInfo> | undefined; + let paneMap: Map<string, PaneInfo> | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/kill.ts` at line 23, Extract the inline type import by adding a top-level type import for PaneInfo and then use that named type for the paneMap declaration; specifically, import { PaneInfo } from the module currently referenced inline (replace the inline import in the declaration of paneMap) and change the variable declaration to use Map<string, PaneInfo> | undefined so the code uses the named PaneInfo type instead of the inline import.windows/PPGDesktop/MainWindow.xaml (2)
88-98: Consider using StaticResource for status indicator colors.The connection indicator uses hard-coded hex colors (
#9ca3affor disconnected,#34d399for connected) while other parts of the UI useStaticResourcebrushes. For theme consistency and maintainability, consider defining these as named resources.♻️ Optional: Define status colors as resources
Add to a resource dictionary:
<SolidColorBrush x:Key="StatusDisconnectedBrush" Color="#9ca3af" /> <SolidColorBrush x:Key="StatusConnectedBrush" Color="#34d399" />Then update the style:
<Setter Property="Fill" Value="{StaticResource StatusDisconnectedBrush}" /> ... <Setter Property="Fill" Value="{StaticResource StatusConnectedBrush}" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@windows/PPGDesktop/MainWindow.xaml` around lines 88 - 98, Replace the hard-coded hex colors in the Ellipse.Style (the Style targeting Ellipse with the Setter Property="Fill" and the DataTrigger binding to ConnectionState) with StaticResource brushes: define SolidColorBrush resources named e.g. StatusDisconnectedBrush and StatusConnectedBrush in your ResourceDictionary, then change the base Setter Fill to "{StaticResource StatusDisconnectedBrush}" and the DataTrigger Setter Fill to "{StaticResource StatusConnectedBrush}" so the indicator follows theme resources.
124-131: Hard-coded hover/press colors in ToolbarButton style.Similar to the status indicator, the button hover (
#3f3f46) and press (#52525b) colors are hard-coded. If theme support is planned, these should be extracted to resources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@windows/PPGDesktop/MainWindow.xaml` around lines 124 - 131, The ToolbarButton ControlTemplate currently hard-codes hover and pressed colors in the ControlTemplate.Triggers (Trigger Property="IsMouseOver" Value="True" and Trigger Property="IsPressed" Value="True"); change these Setters to use themeable resource keys (e.g., DynamicResource or StaticResource like ToolbarButtonHoverBrush and ToolbarButtonPressedBrush) instead of literal hex values, and add those brush resources to the resource dictionary (or existing theme resource files) so themes can override them.src/server/routes/spawn.ts (1)
64-73: Status mapping could be more comprehensive.The
statusForPpgErrorfunction only handlesINVALID_ARGSandNOT_INITIALIZED. Other error codes likeTMUX_NOT_FOUNDorTEMPLATE_NOT_FOUND(mentioned in the AI summary) will fall through to 500, which may not be the most appropriate status. Consider aligning withmapErrorToStatusinagents.ts.♻️ Suggested expansion
function statusForPpgError(code: string): number { switch (code) { case 'INVALID_ARGS': return 400; case 'NOT_INITIALIZED': - return 409; + return 503; + case 'TMUX_NOT_FOUND': + return 503; + case 'TEMPLATE_NOT_FOUND': + return 404; default: return 500; } }Note:
NOT_INITIALIZEDreturning 409 (Conflict) is debatable; 503 (Service Unavailable) may be more semantically correct, as used inagents.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/spawn.ts` around lines 64 - 73, statusForPpgError currently only maps 'INVALID_ARGS' and 'NOT_INITIALIZED' to HTTP statuses; expand this function (statusForPpgError) to include the other PPG error codes referenced in the repo (for example 'TMUX_NOT_FOUND' and 'TEMPLATE_NOT_FOUND' -> 404) and align the semantics with mapErrorToStatus in agents.ts (e.g., change or add handling for 'NOT_INITIALIZED' to return 503 instead of 409 if you want consistency). Update statusForPpgError's switch to explicitly map all known error codes used by the ppg layer to the corresponding HTTP codes used in mapErrorToStatus and keep a default 500 fallback. Ensure you reference and mirror the mappings defined in agents.ts so the two functions remain consistent.src/server/routes/agents.ts (1)
211-288: Potential duplication withperformRestartinsrc/core/operations/restart.ts.The restart route (lines 230-283) manually implements prompt resolution, config loading, and calls
restartAgent, which duplicates logic inperformRestartfromsrc/core/operations/restart.ts. Consider delegating toperformRestartfor consistency, as the kill command now delegates toperformKill.♻️ Suggested refactor to use performRestart
+import { performRestart } from '../../core/operations/restart.js'; // In the restart route handler: - const manifest = await requireManifest(projectRoot); - const config = await loadConfig(projectRoot); - - const found = findAgent(manifest, id); - if (!found) throw new AgentNotFoundError(id); - - const { worktree: wt, agent: oldAgent } = found; - - // Read original prompt or use override - let promptText: string; - if (promptOverride) { - promptText = promptOverride; - } else { - const pFile = agentPromptFile(projectRoot, oldAgent.id); - try { - promptText = await fs.readFile(pFile, 'utf-8'); - } catch { - throw new PpgError( - `Could not read original prompt for agent ${oldAgent.id}. Provide a prompt in the request body.`, - 'PROMPT_NOT_FOUND', - ); - } - } - - const agentConfig = resolveAgentConfig(config, agentType ?? oldAgent.agentType); - - const result = await restartAgent({ - projectRoot, - agentId: oldAgent.id, - worktree: wt, - oldAgent, - sessionName: manifest.sessionName, - agentConfig, - promptText, - }); + const result = await performRestart({ + agentRef: id, + prompt: promptOverride, + agentType, + projectRoot, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/agents.ts` around lines 211 - 288, The route handler duplicates restart logic; replace the manual manifest/config loading, prompt resolution, and call to restartAgent with a call to performRestart to centralize behavior (similar to how performKill is used). Locate the POST /agents/:id/restart handler and remove the inline use of requireManifest, loadConfig, prompt file reading (agentPromptFile), resolveAgentConfig and restartAgent; instead call performRestart with the same inputs (projectRoot, agent id, optional prompt override, optional agent type) and map its result into the existing response shape, preserving AgentNotFoundError and PpgError handling and using existing helpers like findAgent only if needed for pre-validation. Ensure performRestart is imported from src/core/operations/restart.ts and return the newAgent fields from performRestart’s output.src/server/ws/watcher.ts (2)
86-91: Same silent error pattern on directory watcher.Line 88 also swallows errors. For consistency, consider forwarding to
onError.🔧 Proposed fix
- dirWatcher.on('error', () => {}); + dirWatcher.on('error', (err) => onError?.(err));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/ws/watcher.ts` around lines 86 - 91, The directory watcher currently swallows errors in dirWatcher.on('error', () => {}); — change it to forward the error to the existing error handler (call onError with the error) so directory watcher failures are surfaced consistently; locate dirWatcher in watcher.ts and replace the empty error handler with a callback that passes the error to onError (or the appropriate error-dispatching function used elsewhere in this file).
65-73: Swallowedfs.watcherrors may hide issues.The error handler on line 68 silently ignores all watcher errors. Consider forwarding these to the
onErrorcallback to aid debugging, especially for permission issues or watch limit exhaustion.🔧 Proposed fix
function watchManifestFile(): boolean { try { fileWatcher = fs.watch(mPath, onFsChange); - fileWatcher.on('error', () => {}); + fileWatcher.on('error', (err) => onError?.(err)); return true; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/ws/watcher.ts` around lines 65 - 73, watchManifestFile currently attaches an 'error' handler to fileWatcher that swallows errors; update the handler on the fileWatcher created in watchManifestFile (and keep the try/catch) to accept the error parameter and call the module's onError callback (or propagate it) instead of ignoring it so permission/watch-limit and other fs.watch errors are surfaced; reference fileWatcher, watchManifestFile, onFsChange and onError when making the change.ios/PPGMobile/PPGMobile/Views/Dashboard/WorktreeDetailView.swift (3)
26-45: Silent error handling may leave users without feedback.Lines 28 and 39 use
try?which silently discards errors. IfmergeWorktreeorkillWorktreefails, the user sees no error message and the UI simply refreshes to the unchanged state.Consider propagating errors to
appState.errorMessagefor user feedback, similar to howAppState.killAgenthandles errors (per the relevant snippet fromAppState.swift).🔧 Example fix for merge action
Button("Squash Merge") { Task { - try? await appState.client.mergeWorktree(worktreeId: worktreeId) - await appState.manifestStore.refresh() + do { + try await appState.client.mergeWorktree(worktreeId: worktreeId) + await appState.manifestStore.refresh() + } catch { + appState.errorMessage = "Merge failed: \(error.localizedDescription)" + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Views/Dashboard/WorktreeDetailView.swift` around lines 26 - 45, In WorktreeDetailView replace the silent `try? await` calls for appState.client.mergeWorktree(worktreeId:) and appState.client.killWorktree(worktreeId:) with explicit do/catch blocks: call the async method inside a do, await appState.manifestStore.refresh() on success, and in the catch set appState.errorMessage (or a localized-friendly string) to the caught error’s description so the user gets feedback; reference the Button actions that currently call mergeWorktree and killWorktree and mirror the error-handling pattern used by AppState.killAgent.
149-156: Same silent error pattern for PR creation.Line 151 uses
try?which discards PR creation errors. Consider the same error handling pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Views/Dashboard/WorktreeDetailView.swift` around lines 149 - 156, The Task currently swallows errors from appState.client.createPR by using try?, so change it to use do/catch: call try await appState.client.createPR(worktreeId: worktreeId) inside a do block, await appState.manifestStore.refresh() on success, and in the catch block log the error and surface it to the user (e.g., via your existing appState error/alert mechanism) so creation failures are not silently ignored; update the Button's Task closure around createPR and manifestStore.refresh accordingly.
108-117: Same silent error pattern for agent restart.Line 113 uses
try?which discards restart errors. Consider the same error handling pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/PPGMobile/PPGMobile/Views/Dashboard/WorktreeDetailView.swift` around lines 108 - 117, The onRestart closure currently swallows errors with try? when calling appState.client.restartAgent(agentId:), so change it to use the same explicit error-handling pattern as onKill: wrap the restart call in a Task containing a do { try await appState.client.restartAgent(agentId: agent.id); await appState.manifestStore.refresh() } catch { await appState.handleError(error) } (or, if you already have an appState.restartAgent(agent.id) helper that handles errors, call that instead) so restart failures are surfaced rather than silently discarded.src/core/operations/spawn.test.ts (1)
21-32: Align tmux/git test doubles with the repo’s mock contract.These tests fully replace
worktree/tmuxmodules instead of mockingexecaand partially mocking modules, which can hide integration regressions at the command boundary.As per coding guidelines,
src/**/*.test.ts: “Mockexecafor tmux/git calls and usevi.mockwithvi.importActualfor partial mocks in tests”.Also applies to: 47-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/operations/spawn.test.ts` around lines 21 - 32, Tests currently fully replace the worktree/tmux modules which hides command-boundary regressions; instead, switch to partial module mocks using vi.mock(..., async () => ({ ...await vi.importActual('../worktree.js'), <overrideFns> })) for worktree and tmux so real implementations stay intact except for the specific functions you override (e.g., readManifest, updateManifest, resolveWorktree, getRepoRoot, getCurrentBranch, createWorktree, adoptWorktree), and stop stubbing tmux/git behavior directly—mock execa (the command runner) to simulate tmux/git CLI results in spawn.test.ts so the tests exercise the module integration while controlling external command outputs.src/server/tls.ts (1)
335-348: Avoid duplicating pairing URL assembly logic.
buildPairingUrlhere overlaps withsrc/commands/serve.ts(same protocol/query contract). Consolidating into one shared helper reduces drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/tls.ts` around lines 335 - 348, The pairing URL assembly implemented by buildPairingUrl duplicates the same logic used in the serve command; extract this logic into a single exported helper (preserve the signature: buildPairingUrl(params: {host,string; port:number; caFingerprint:string; token:string}): string) in a shared module and replace the local implementation here and in the serve command with imports to that helper, updating all callers to use the shared buildPairingUrl so the ppg://connect?host=...&port=...&ca=...&token=... contract remains consistent.
Comment on lines
+39
to
+45
| final class WebSocketManager: NSObject, @unchecked Sendable, URLSessionWebSocketDelegate { | ||
|
|
||
| // MARK: - Callbacks | ||
|
|
||
| var onStateChange: ((WebSocketConnectionState) -> Void)? | ||
| var onEvent: ((WebSocketEvent) -> Void)? | ||
| var onMessage: ((ServerMessage) -> Void)? |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect callback declarations/usages in WebSocketManager ==="
rg -n --type=swift -C2 '@unchecked Sendable|onStateChange|onEvent|onMessage|queue\.async|DispatchQueue\.main\.async' ios/PPGMobile/PPGMobile/Networking/WebSocketManager.swift
echo ""
echo "=== Find callback assignments in app code ==="
rg -n --type=swift -C2 '\.onStateChange\s*=|\.onEvent\s*=|\.onMessage\s*=' ios/PPGMobile
echo ""
echo "=== Check whether assignment sites are actor/queue constrained ==="
rg -n --type=swift -C3 '@MainActor|MainActor\.run|DispatchQueue\.main' ios/PPGMobileRepository: 2witstudios/ppg-cli
Length of output: 13346
Synchronize callback storage on one executor (or actor-isolate this type).
With @unchecked Sendable, mutable callback vars are shared state accessed from different queues without synchronization. Callbacks are written on the main thread (within @MainActor contexts in AppState and TerminalView) but read on the internal queue (lines 177, 229, 237). While the reads capture to a local variable before invocation, the var itself is still subject to data races under concurrent access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/PPGMobile/PPGMobile/Networking/WebSocketManager.swift` around lines 39 -
45, The mutable callback vars onStateChange/onEvent/onMessage in
WebSocketManager are currently unsynchronized (class marked `@unchecked` Sendable)
and are written from MainActor contexts but read on the internal queue, risking
data races; fix by making callback storage exclusively owned by the internal
queue: make the vars private (e.g. private var onStateChange/onEvent/onMessage)
and provide setter methods (or public properties) that dispatch writes to the
internal queue (using queue.async/queue.sync as appropriate), leaving all reads
on the same queue so all access to those symbols is queue-synchronized;
alternatively, actor-isolate the whole type (remove `@unchecked` Sendable and
annotate WebSocketManager with `@MainActor`) if you want main-thread-only access
instead.
Comment on lines
+101
to
+114
| private func addServer() { | ||
| guard let validatedPort = parsedPort else { return } | ||
| let connection = ServerConnection( | ||
| name: trimmedName.isEmpty ? "My Mac" : trimmedName, | ||
| host: trimmedHost, | ||
| port: validatedPort, | ||
| token: trimmedToken | ||
| ) | ||
| appState.addConnection(connection) | ||
| Task { | ||
| await appState.connect(to: connection) | ||
| } | ||
| dismiss() | ||
| } |
There was a problem hiding this comment.
Dismiss called before connection completes — user may miss errors.
The view dismisses immediately after starting the connection task, so users won't see the errorMessage set by connect(to:) if the connection fails. Consider awaiting the connection result before dismissing, or showing errors via a toast/alert on the parent view.
💡 Proposed fix to await connection result
private func addServer() {
guard let validatedPort = parsedPort else { return }
let connection = ServerConnection(
name: trimmedName.isEmpty ? "My Mac" : trimmedName,
host: trimmedHost,
port: validatedPort,
token: trimmedToken
)
appState.addConnection(connection)
Task {
await appState.connect(to: connection)
+ await MainActor.run { dismiss() }
}
- dismiss()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/PPGMobile/PPGMobile/Views/Settings/AddServerView.swift` around lines 101
- 114, In addServer(), do not call dismiss() immediately after spawning the
connection Task; instead await the result of appState.connect(to:) and only
dismiss on success, otherwise keep the view visible and surface the failure
(e.g., via appState.errorMessage or an alert/toast). Concretely: after
appState.addConnection(connection), call await appState.connect(to: connection)
inside the Task, check the returned success/failure (or the error state) from
connect(to:) and call dismiss() only when connection succeeds; if it fails,
present the error via the existing errorMessage or trigger a UI alert so the
user can see and correct the issue. Ensure references to addServer(),
appState.addConnection(_:), and appState.connect(to:) are used to locate and
update the logic.
Comment on lines
+42
to
+46
| .task { await viewModel.subscribe(agentId: agentId, appState: appState) } | ||
| .onDisappear { | ||
| if let ws = appState.wsManager { | ||
| viewModel.unsubscribe(agentId: agentId, wsManager: ws) | ||
| } |
There was a problem hiding this comment.
Fix subscription lifecycle when wsManager is nil to avoid stuck state and missed cleanup.
subscribe marks isSubscribed before confirming WS availability, and onDisappear skips unsubscribe when wsManager is already nil. That can leave the view model in a false “subscribed” state and bypass router cleanup.
💡 Proposed fix
`@Observable`
`@MainActor`
final class TerminalViewModel {
var output = ""
var hasError = false
private(set) var isSubscribed = false
private static let maxOutputLength = 50_000
private var subscriptionID: UUID?
+ private weak var subscribedWSManager: WebSocketManager?
func subscribe(agentId: String, appState: AppState) async {
guard !isSubscribed else { return }
- isSubscribed = true
// Fetch initial log content via REST
do {
let logs = try await appState.client.fetchAgentLogs(agentId: agentId, lines: 200)
output = logs.output
trimOutput()
} catch {
output = "Failed to load logs: \(error.localizedDescription)"
hasError = true
}
// Subscribe to live WebSocket updates
- guard let ws = appState.wsManager else { return }
+ guard let ws = appState.wsManager else {
+ isSubscribed = false
+ return
+ }
+ isSubscribed = true
+ subscribedWSManager = ws
subscriptionID = TerminalMessageRouter.shared.addSubscriber(wsManager: ws) { [weak self] message in
guard message.type == "terminal:output", message.agentId == agentId, let data = message.data else {
return
}
Task { `@MainActor` [weak self] in
guard let self else { return }
self.output += data
self.trimOutput()
}
}
ws.subscribeTerminal(agentId: agentId)
}
- func unsubscribe(agentId: String, wsManager: WebSocketManager) {
+ func unsubscribe(agentId: String, wsManager: WebSocketManager? = nil) {
guard isSubscribed else { return }
isSubscribed = false
- wsManager.unsubscribeTerminal(agentId: agentId)
+ let ws = wsManager ?? subscribedWSManager
+ ws?.unsubscribeTerminal(agentId: agentId)
if let subscriptionID {
- TerminalMessageRouter.shared.removeSubscriber(wsManager: wsManager, subscriberID: subscriptionID)
+ if let ws {
+ TerminalMessageRouter.shared.removeSubscriber(wsManager: ws, subscriberID: subscriptionID)
+ }
self.subscriptionID = nil
}
+ subscribedWSManager = nil
}
}- .onDisappear {
- if let ws = appState.wsManager {
- viewModel.unsubscribe(agentId: agentId, wsManager: ws)
- }
- }
+ .onDisappear {
+ viewModel.unsubscribe(agentId: agentId, wsManager: appState.wsManager)
+ }Also applies to: 117-133, 146-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/PPGMobile/PPGMobile/Views/Terminal/TerminalView.swift` around lines 42 -
46, The subscribe flow sets viewModel.isSubscribed before confirming
appState.wsManager exists, and onDisappear skips unsubscribe when wsManager is
nil—fix by making subscribe(agentId:appState:) only mark isSubscribed after a
successful WS subscription (i.e., check/apply appState.wsManager != nil inside
subscribe and return early on failure), and make unsubscribe(agentId:wsManager:)
robust to a nil wsManager by performing local cleanup (clearing isSubscribed and
router/VM state) even when wsManager is nil; then change TerminalView’s
.onDisappear to always call viewModel.unsubscribe(agentId: agentId, wsManager:
appState.wsManager) (or call a new viewModel.forceUnsubscribe(agentId:) that
performs cleanup) so cleanup never gets skipped.
Comment on lines
+291
to
+306
| describe('serveDaemonCommand', () => { | ||
| test('given initialized project, should call runServeDaemon with correct args', async () => { | ||
| await serveDaemonCommand({ port: 4000, host: '0.0.0.0' }); | ||
|
|
||
| expect(requireManifest).toHaveBeenCalledWith('/fake/project'); | ||
| expect(runServeDaemon).toHaveBeenCalledWith('/fake/project', 4000, '0.0.0.0'); | ||
| }); | ||
|
|
||
| test('given project not initialized, should throw', async () => { | ||
| const err = Object.assign(new Error('Not initialized'), { code: 'NOT_INITIALIZED' }); | ||
| vi.mocked(requireManifest).mockRejectedValue(err); | ||
|
|
||
| await expect(serveDaemonCommand({ port: 3000, host: 'localhost' })).rejects.toThrow('Not initialized'); | ||
| expect(runServeDaemon).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test expects runServeDaemon but implementation calls startServer.
The test at line 296 asserts runServeDaemon was called with specific arguments, but the actual implementation in serve.ts (line 200) calls startServer instead. This test will fail.
Either the implementation should call runServeDaemon, or the test should mock and verify startServer.
🐛 If the implementation is correct, update the test
vi.mock('../core/serve.js', async (importOriginal) => {
const actual = await importOriginal() as Record<string, unknown>;
return {
...actual,
isServeRunning: vi.fn(() => false),
getServePid: vi.fn(() => null),
getServeInfo: vi.fn(() => null),
readServeLog: vi.fn(() => []),
- runServeDaemon: vi.fn(),
};
});
+
+vi.mock('../server/index.js', () => ({
+ startServer: vi.fn(),
+}));
// Then in the test:
+const { startServer } = await import('../server/index.js');
describe('serveDaemonCommand', () => {
test('given initialized project, should call runServeDaemon with correct args', async () => {
await serveDaemonCommand({ port: 4000, host: '0.0.0.0' });
expect(requireManifest).toHaveBeenCalledWith('/fake/project');
- expect(runServeDaemon).toHaveBeenCalledWith('/fake/project', 4000, '0.0.0.0');
+ expect(startServer).toHaveBeenCalledWith({
+ projectRoot: '/fake/project',
+ port: 4000,
+ host: '0.0.0.0',
+ token: undefined,
+ });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/serve.test.ts` around lines 291 - 306, The test asserts
runServeDaemon was called but the implementation in serveDaemonCommand calls
startServer; update the test to mock/expect startServer (replace
expect(runServeDaemon)... with expect(startServer)... and ensure startServer is
properly imported/mocked), or alternatively change serveDaemonCommand to call
runServeDaemon instead of startServer so the test and implementation match;
locate serveDaemonCommand, requireManifest, runServeDaemon and startServer to
make the corresponding change.
Comment on lines
+197
to
+201
| export async function serveDaemonCommand(options: { port: number; host: string; token?: string }): Promise<void> { | ||
| const projectRoot = await getRepoRoot(); | ||
| await requireManifest(projectRoot); | ||
| await startServer({ projectRoot, port: options.port, host: options.host, token: options.token }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if runServeDaemon and startServer exist
rg -n "export.*function startServer" --type ts
rg -n "export.*function runServeDaemon" --type tsRepository: 2witstudios/ppg-cli
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check the actual implementation of serveDaemonCommand
sed -n '195,205p' src/commands/serve.ts
# Check what the test expects
rg -n -A 10 "expect.*runServeDaemon\|expect.*startServer" --type ts src/commands/serve.test.ts | head -30Repository: 2witstudios/ppg-cli
Length of output: 363
🏁 Script executed:
#!/bin/bash
# Find and check the test file
fd "serve.test.ts" --type f
# Check the test around line 296
if [ -f "src/commands/serve.test.ts" ]; then
sed -n '290,310p' src/commands/serve.test.ts
fiRepository: 2witstudios/ppg-cli
Length of output: 1002
Implementation/test mismatch: serveDaemonCommand calls startServer, but the test expects runServeDaemon.
The function at line 200 calls startServer, but the test at src/commands/serve.test.ts line 296 asserts that runServeDaemon was called. One of these must be corrected to match the intended implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/serve.ts` around lines 197 - 201, The implementation calls
startServer but tests expect runServeDaemon; update serveDaemonCommand to call
runServeDaemon with the same arguments (projectRoot, port, host, token) instead
of startServer and ensure runServeDaemon is imported/available where
serveDaemonCommand is defined so the function signature and behavior match the
test's expectation.
Comment on lines
+141
to
+144
| const token = url.searchParams.get('token'); | ||
| if (!token) { | ||
| rejectUpgrade(socket, 'HTTP/1.1 401 Unauthorized'); | ||
| return; |
There was a problem hiding this comment.
Avoid query-string bearer tokens for WebSocket auth.
Line 141 accepts token from the URL query, which is commonly captured in logs and intermediaries. Prefer header/subprotocol or short-lived one-time upgrade tokens, with query-token support only as a compatibility fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/ws/handler.ts` around lines 141 - 144, The handler currently reads
bearer tokens from the URL query (url.searchParams.get('token')) which is
insecure; change the authentication order in the WebSocket upgrade handling (in
the same handler function in src/server/ws/handler.ts) to first look for an
Authorization header (or Sec-WebSocket-Protocol subprotocol if your client uses
that) for the bearer token, validate that, and only fall back to
url.searchParams.get('token') if no header/subprotocol token is present and the
token is known to be short-lived/one-time; when falling back emit a
deprecation/warning log line so usage is visible. Keep the rejection behavior
using rejectUpgrade unchanged when no token is found or validation fails.
Comment on lines
+86
to
+99
| export class TerminalStreamer { | ||
| private streams = new Map<string, AgentStream>(); | ||
| private nextSubscriberId = 1; | ||
| private readonly pollIntervalMs: number; | ||
| /** Injectable capture function — defaults to tmux capturePane. */ | ||
| private readonly capture: (target: string, lines?: number) => Promise<string>; | ||
|
|
||
| constructor(options?: { | ||
| pollIntervalMs?: number; | ||
| capture?: (target: string, lines?: number) => Promise<string>; | ||
| }) { | ||
| this.pollIntervalMs = options?.pollIntervalMs ?? POLL_INTERVAL_MS; | ||
| this.capture = options?.capture ?? capturePane; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor TerminalStreamer away from class-based architecture.
This file is under src/**/*.ts, where repository rules require functional style and disallow classes except PpgError.
As per coding guidelines, "Use functional style: pure functions, composition, const declarations, destructuring, and no classes except PpgError."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/ws/terminal.ts` around lines 86 - 99, The TerminalStreamer class
violates the repository rule against classes; refactor it into a functional
module exposing equivalent functions and state held in const closures: replace
the class TerminalStreamer with a factory function (e.g.,
createTerminalStreamer) that returns an API object with methods corresponding to
the class methods and uses internal const-closed variables for streams
(Map<string,AgentStream>), nextSubscriberId, pollIntervalMs (defaulting to
POLL_INTERVAL_MS), and capture (defaulting to capturePane). Ensure all usages of
the constructor and instance methods are updated to call
createTerminalStreamer(...) and its returned methods, preserve types for
AgentStream and the capture signature, and export only the factory and its API
so no class remains except PpgError.
Comment on lines
+179
to
+183
| private scheduleNextPoll(agentId: string, stream: AgentStream): void { | ||
| stream.timer = setTimeout(() => { | ||
| void this.poll(agentId, stream); | ||
| }, this.pollIntervalMs); | ||
| } |
There was a problem hiding this comment.
Protect against stale in-flight polls deleting a newer stream.
A previous poll() can outlive unsubscribe/re-subscribe cycles for the same agentId; current catch cleanup can remove the newly created stream entry.
🛠️ Suggested guard to keep cleanup scoped to the active stream instance
private async poll(agentId: string, stream: AgentStream): Promise<void> {
+ if (this.streams.get(agentId) !== stream) return;
try {
const raw = await this.capture(stream.tmuxTarget);
const currentLines = raw.split('\n');
@@
- if (stream.subscribers.size > 0) {
+ if (this.streams.get(agentId) === stream && stream.subscribers.size > 0) {
this.scheduleNextPoll(agentId, stream);
}
} catch (err) {
+ if (this.streams.get(agentId) !== stream) return;
// Pane gone / tmux error — notify subscribers and clean up
const errorMsg = JSON.stringify({
type: 'terminal:error',
agentId,
error: 'Pane no longer available',
} satisfies TerminalError);
@@
stream.timer = null;
stream.subscribers.clear();
this.streams.delete(agentId);
}
}Also applies to: 214-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/ws/terminal.ts` around lines 179 - 183, The scheduled poll can
outlive unsubscribe/re-subscribe and accidentally clean up a new stream; update
poll-related cleanup logic (used by scheduleNextPoll, poll, and the similar
block around lines 214-238) to guard against deleting the active stream by
comparing the currently stored stream against the captured instance before
modifying or deleting it (e.g., check this.streams.get(agentId) === stream) and
only clear timers or remove the entry when the stored reference matches the
captured AgentStream instance.
Comment on lines
+187
to
+188
| const raw = await this.capture(stream.tmuxTarget); | ||
| const currentLines = raw.split('\n'); |
There was a problem hiding this comment.
Bound capture size; full-pane polling every tick is unbounded.
Polling currently captures the entire pane buffer each cycle. Under long-running sessions this creates avoidable CPU/IO overhead.
⚙️ Suggested bounded capture
const POLL_INTERVAL_MS = 500;
+const CAPTURE_LINES = 300;
@@
- const raw = await this.capture(stream.tmuxTarget);
+ const raw = await this.capture(stream.tmuxTarget, CAPTURE_LINES);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/ws/terminal.ts` around lines 187 - 188, The polling currently
calls this.capture(stream.tmuxTarget) and splits the entire pane buffer, which
is unbounded; change to a bounded capture by limiting the number of lines read
each tick (e.g., capture only the last N lines) and/or by changing the capture
API call to accept start/end or maxLines (e.g., this.capture(stream.tmuxTarget,
{ maxLines: 200 })) and then operate on that slice instead of raw.split('\n');
alternatively implement incremental polling in the surrounding class (track
lastSeenLine or lastCaptureIndex on the stream object and request only newer
content) so Terminal polling methods (capture and the consumer that uses
currentLines) never pull the full pane buffer each cycle.
Comment on lines
+18
to
+23
| // Bool with format parameter "TrueText|FalseText" | ||
| if (value is bool bVal && parameter is string format && format.Contains('|')) | ||
| { | ||
| var parts = format.Split('|'); | ||
| return bVal ? parts[1] : parts[0]; | ||
| } |
There was a problem hiding this comment.
"TrueText|FalseText" mapping is reversed in Convert.
At Line 22, true returns parts[1] and false returns parts[0], which is opposite of the documented format and will swap UI labels/states.
💡 Proposed fix
- return bVal ? parts[1] : parts[0];
+ return bVal ? parts[0] : parts[1];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Bool with format parameter "TrueText|FalseText" | |
| if (value is bool bVal && parameter is string format && format.Contains('|')) | |
| { | |
| var parts = format.Split('|'); | |
| return bVal ? parts[1] : parts[0]; | |
| } | |
| // Bool with format parameter "TrueText|FalseText" | |
| if (value is bool bVal && parameter is string format && format.Contains('|')) | |
| { | |
| var parts = format.Split('|'); | |
| return bVal ? parts[0] : parts[1]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@windows/PPGDesktop/Converters/InverseBoolConverter.cs` around lines 18 - 23,
The Convert implementation in InverseBoolConverter reverses the
"TrueText|FalseText" mapping: when parameter is a string format containing '|'
and split into parts, swap the returned indices so that true returns parts[0]
and false returns parts[1]; update the conditional branch (the block that checks
"if (value is bool bVal && parameter is string format && format.Contains('|'))"
and uses the parts variable) to return parts[0] for bVal == true and parts[1]
for false.
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
1 participant
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
ppg servevia REST API + WebSocket for real-time updatesArchitecture
42 files, ~3000 lines across:
Key Features
Prerequisites
download-xterm.ps1to install xterm.js dependenciesTest plan
dotnet buildcompiles cleanly on Windows with .NET 8download-xterm.ps1to install xterm.js, verify terminal rendersppg serveinstanceSummary by CodeRabbit
Release Notes
New Features
servecommand to start/stop a persistent PPG server with optional token-based authentication and pairing URL generation.Refactoring
Lines Changed: +7,700 (substantial new codebase)