feat(electron): add opencode-backed coding workspace shell#9
feat(electron): add opencode-backed coding workspace shell#9
Conversation
- add an exploration for in-app app development across Electron, web, and mobile - map current repo support for devtools history, Local API, MCP, plugins, and platform capabilities - recommend an Electron-first workspace-shell plus preview-worktree architecture with PR automation
- add an Electron-first exploration for a Codex-like xNet workspace shell - compare OpenCode integration paths, preview surface options, and git/PR orchestration - add performance-first guidance for instant session switching, warm previews, and local xNet-backed state
- add an MVP-focused exploration for a fast self-editing Electron workspace - recommend reusing OpenCode web as the primary chat UI and coding-agent substrate - detail cheap performance wins built on xNet React/data primitives, warm previews, and thin git worktree wrappers - compare OpenCode with T3 Code and Vercel's coding-agent template using current official sources
- add plan03_9_83LLMCodingUI as a six-step Electron MVP implementation plan - structure the rollout around OpenCode reuse, xNet-backed fast shell state, git worktrees, previews, and PR drafting - document current repo touchpoints, official OpenCode and Electron guidance, and MVP performance guardrails
- align the preload service bridge with the full service IPC contract - fix service listener teardown and add OpenCode host status and control APIs - add local OpenCode lifecycle management, localhost CSP support, and Step 01 progress
- add app-local xNet schemas for coding session summaries and shell selection state - add workspace hooks and bootstrap wiring for denormalized summaries and active-session restore - add renderer tests and update the LLM coding UI plan checklist for Step 02
- add a three-panel coding workspace mode with a session rail and OpenCode center panel - wire the canvas shell menu and command palette into the workspace entry point - add renderer tests and update the Step 03 plan checklist progress
- add a workspace-session IPC bridge with thin git CLI wrappers and warm preview management - wire the renderer shell to create, sync, refresh, restart, and remove real worktree sessions - persist preview and git status back into xNet session summaries and check off Step 04 plan tasks
- add a preview-to-shell context bridge, OpenCode prompt prefill, and selected-context file output - replace placeholder review tabs with git-backed files, diff, markdown, screenshot, and PR actions - tag preview surfaces with xnet target metadata and check off Step 05 plan tasks
- add timing telemetry for session switching, preview restore, OpenCode readiness, review generation, screenshots, and PR flows - surface dirty worktrees and runtime errors in the renderer shell, and add actionable command recovery messages in Electron main - document Step 06 hardening, validation results, and native rebuild recovery paths for the Electron MVP
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a coding workspace to the Electron app: OpenCode host lifecycle and IPC, GitService for worktrees, PreviewManager for per-session previews, renderer three‑panel workspace UI with hooks/panels, preload IPC surface and access control, shared workspace-session contracts, many tests, tsconfig/vite/test adjustments, and extensive docs and plans. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer
participant Preload as Preload Bridge
participant MainIPC as Main IPC
participant OpenCodeCtrl as OpenCode Controller
participant ProcessMgr as Process Manager
participant Binary as Binary Resolver
participant Health as Health Prober
Renderer->>Preload: xnetOpenCode.ensure()
Preload->>MainIPC: opencode:ensure (IPC)
MainIPC->>OpenCodeCtrl: ensure()
OpenCodeCtrl->>Binary: resolveBinary(config)
Binary-->>OpenCodeCtrl: {found, path}
alt binary found
OpenCodeCtrl->>ProcessMgr: startService(def)
ProcessMgr-->>OpenCodeCtrl: service status
OpenCodeCtrl->>Health: probeHealth()
Health-->>OpenCodeCtrl: health payload
OpenCodeCtrl-->>MainIPC: publish ready status + version
else missing binary
OpenCodeCtrl-->>MainIPC: publish missing-binary status + recovery
end
MainIPC-->>Preload: OpenCodeHostStatus
Preload-->>Renderer: Promise resolved (status)
sequenceDiagram
participant UI as Renderer UI
participant Cmds as useSessionCommands
participant WorkspaceIPC as workspace-session-ipc
participant GitSvc as GitService
participant PreviewMgr as PreviewManager
participant Storage as xNet Storage
UI->>Cmds: createWorkspaceSession()
Cmds->>Storage: create SessionSummary
Storage-->>Cmds: summaryNode
Cmds->>WorkspaceIPC: xnetWorkspaceSessions.create(input)
WorkspaceIPC->>GitSvc: createWorktree(branch, session)
GitSvc-->>WorkspaceIPC: CreatedWorktree
WorkspaceIPC->>PreviewMgr: ensureSession(descriptor)
PreviewMgr-->>WorkspaceIPC: preview status
WorkspaceIPC-->>Cmds: WorkspaceSessionSnapshot
Cmds->>Storage: applySnapshot(patch)
Storage-->>UI: updated summaries
sequenceDiagram
participant Preview as Embedded Preview
participant Bridge as PreviewContextBridge
participant Renderer as Renderer
participant Hook as usePreviewSelectedContext
participant Store as File Store
participant OpenCodeAPI as xnetOpenCode
Preview->>Bridge: user right-click -> postMessage(selectedContext)
Bridge-->>Renderer: window.postMessage(selectedContext)
Renderer->>Hook: message received
Hook->>Store: storeWorkspaceSelectedContext(selectedContext)
Store-->>Hook: path
Hook->>Hook: buildSelectedContextPrompt(path)
Hook->>OpenCodeAPI: appendPrompt(prompt)
OpenCodeAPI-->>Hook: { ok: true }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Preview: https://xnet.fyi/pr/9/app/ |
Greptile SummaryThis PR introduces a substantial Electron coding workspace MVP (~10 000 lines) that wires together a three-panel shell (session rail, embedded OpenCode WebView, and preview/review surface), git worktree lifecycle management, a Vite preview manager, and a context-bridge for right-click element capture. The feature set is well-scoped and architecturally sensible, but a number of correctness and security issues exist across the newly introduced main-process services that should be resolved before shipping. Key issues carried over from previous review rounds that remain unaddressed:
New issues found in this review:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer
participant P as Preload (contextBridge)
participant IPC as workspace-session-ipc
participant Git as GitService
participant PM as PreviewManager
participant OC as OpenCodeService
participant OCCtrl as OpenCodeHostController
R->>P: xnetWorkspaceSessions.create(input)
P->>IPC: ipcMain CREATE handler
IPC->>Git: createWorktree(input)
Git-->>IPC: CreatedWorktree
IPC->>PM: ensureSession(descriptor)
PM->>PM: allocatePort (reserves port)
PM->>PM: spawn pnpm exec vite
PM->>PM: waitForReady (poll HTTP)
PM-->>IPC: PreviewRuntimeStatus
IPC->>IPC: buildSessionSnapshot
IPC-->>R: WorkspaceSessionSnapshot
R->>P: xnetOpenCode.appendPrompt(input)
P->>OC: ipcMain APPEND_PROMPT handler
OC->>OCCtrl: controller.ensure()
OCCtrl-->>OC: OpenCodeHostStatus
OC->>OC: waitForOpenCodeHealth (up to 20 polls)
OC->>OC: POST /tui/append-prompt
OC-->>R: {ok: true}
R->>P: xnetWorkspaceSessions.review(input)
P->>IPC: ipcMain REVIEW handler
IPC->>Git: listChangedFiles, getDiffStat, getDiffPatch
Git-->>IPC: file list + diffs (unstaged only)
IPC->>IPC: buildPullRequestDraft (truncates at 10 files)
IPC-->>R: WorkspaceSessionReview
Note over R,PM: Preview context bridge (in iframe)
R->>R: contextmenu → postMessage('*') to parent
R->>P: xnetWorkspaceSessions.storeSelectedContext
P->>IPC: STORE_SELECTED_CONTEXT handler
IPC->>IPC: assertManagedWorktreePath (validates worktreePath)
IPC->>IPC: writeFile .xnet/selected-context.json
Last reviewed commit: d44e9f4 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return pathDirs.flatMap((dir) => getBinaryNames().map((name) => join(dir, name))) | ||
| } | ||
|
|
||
| async function resolveOpenCodeBinary( | ||
| config: OpenCodeHostConfig | ||
| ): Promise<OpenCodeBinaryResolution> { | ||
| const uniqueCandidates = new Set<string>() | ||
|
|
||
| if (config.binaryPathOverride) { | ||
| uniqueCandidates.add(config.binaryPathOverride) |
There was a problem hiding this comment.
ensure() early-returns without starting a stopped service
ensure() skips the start path whenever current && current.state !== 'error'. If the service was previously stopped (i.e., stopOpenCodeHost() was called and the service manager returns { state: 'stopped' }), current would be truthy and current.state !== 'error' would be true, causing ensure() to return refresh() — which reports the service as idle/stopped — rather than restarting it.
After stop() is called, subsequent calls to ensure() from the renderer (e.g., via the "Refresh" button in OpenCodePanel) would silently return a stopped status instead of relaunching the process.
The condition should also skip the early-return for the 'stopped' state:
| return pathDirs.flatMap((dir) => getBinaryNames().map((name) => join(dir, name))) | |
| } | |
| async function resolveOpenCodeBinary( | |
| config: OpenCodeHostConfig | |
| ): Promise<OpenCodeBinaryResolution> { | |
| const uniqueCandidates = new Set<string>() | |
| if (config.binaryPathOverride) { | |
| uniqueCandidates.add(config.binaryPathOverride) | |
| const ensure = async (): Promise<OpenCodeHostStatus> => { | |
| const current = deps.getServiceStatus(OPENCODE_SERVICE_ID) | |
| if (current && current.state !== 'error' && current.state !== 'stopped') { | |
| return refresh() | |
| } |
| }, | ||
| body: JSON.stringify({ | ||
| prompt: input.prompt, | ||
| text: input.prompt | ||
| }) |
There was a problem hiding this comment.
Duplicate prompt/text fields in append-prompt body
Both prompt and text are set to input.prompt, sending the same value twice:
body: JSON.stringify({
prompt: input.prompt,
text: input.prompt // ← same value as prompt
})If the OpenCode API truly requires both fields to be present, a comment explaining which field maps to which API contract would help readers understand this isn't an accident. If only one field is needed, the other can be dropped. If text is meant to carry a different value (e.g., display text vs. injected text), this is a likely copy-paste bug.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/DatabaseView.tsx (1)
447-470:⚠️ Potential issue | 🟡 MinorExpose the selected view state to assistive tech.
These buttons behave like a mode toggle, but the active state is only visual right now. Please add pressed/selected semantics so screen readers can tell which view is active.
♿ Suggested update
<button + type="button" + aria-pressed={viewMode === 'table'} onClick={() => setViewMode('table')} className={`flex items-center gap-1 px-3 py-1.5 text-sm transition-colors ${ viewMode === 'table' ? 'bg-primary text-white' : 'bg-transparent text-foreground hover:bg-accent' }`} data-xnet-target-id="database-table-toggle" data-xnet-target-label="Table view toggle" data-xnet-file-hint="apps/web/src/components/DatabaseView.tsx" > @@ <button + type="button" + aria-pressed={viewMode === 'board'} onClick={() => setViewMode('board')} className={`flex items-center gap-1 px-3 py-1.5 text-sm transition-colors ${ viewMode === 'board' ? 'bg-primary text-white' : 'bg-transparent text-foreground hover:bg-accent' }`} data-xnet-target-id="database-board-toggle" data-xnet-target-label="Board view toggle" data-xnet-file-hint="apps/web/src/components/DatabaseView.tsx" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/DatabaseView.tsx` around lines 447 - 470, The two view-toggle buttons only communicate active state visually; update the button elements (the ones that call setViewMode and read viewMode) to expose the active state to assistive tech by adding an aria-pressed attribute set to a boolean expression (e.g., aria-pressed={viewMode === 'table'} for the Table button and aria-pressed={viewMode === 'board'} for the Board button) so screen readers announce which mode is selected, and ensure the same attributes are applied to the matching button that uses setViewMode('board') and the data-xnet-target-id elements so the behavior is consistent.apps/electron/src/main/index.ts (1)
238-245:⚠️ Potential issue | 🟠 Major
before-quitwill not wait for this teardown.Electron does not delay shutdown for a returned listener Promise here—returning a Promise (or using an
asynchandler) fromapp.on('before-quit', ...)is ignored. Instead, callevent.preventDefault()to prevent the quit, perform async cleanup, then explicitly callapp.quit()with a guard to avoid infinite loops. Without this,stopWorkspaceSessions(),stopOpenCodeHost(), and other async shutdown work can be interrupted mid-cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/index.ts` around lines 238 - 245, The current async handler on app.on('before-quit') will be ignored by Electron; change the listener to accept the event, call event.preventDefault() to stop immediate quit, perform the async teardown (await stopLocalAPI(), stopWorkspaceSessions(), stopOpenCodeHost(), cleanupServices()), then call app.quit() after successful cleanup while using a reentrancy/guard flag (e.g., isQuitting) to avoid infinite quit loops; ensure the guard is checked before preventing default so subsequent app.quit() can proceed.
🧹 Nitpick comments (32)
docs/explorations/0109_[_]_ELECTRON_MVP_SELF_EDITING_WORKSPACE_CHEAP_PERFORMANCE_WINS_OPENCODE_WORKTREES_RIGHT_CLICK_CONTEXT.md (1)
19-22: Consider varying sentence structure for readability.The four consecutive questions all begin with "Can", which creates repetition. While this parallel structure is valid, varying the openings might improve flow.
📝 Suggested rewording
The specific sub-questions are: -- Can OpenCode be reused for most of the LLM/chat/interface complexity? -- Can xNet primitives own the fast local state and timeline UX? -- Can right-clicking the live UI pass useful context into OpenCode with minimal glue code? -- Are there better tools than OpenCode for this repo and this UX? +- Can OpenCode be reused for most of the LLM/chat/interface complexity? +- Will xNet primitives suffice to own the fast local state and timeline UX? +- Is it possible to pass useful context from right-clicking the live UI into OpenCode with minimal glue code? +- Are there better tools than OpenCode for this repo and this UX?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/explorations/0109_`[_]_ELECTRON_MVP_SELF_EDITING_WORKSPACE_CHEAP_PERFORMANCE_WINS_OPENCODE_WORKTREES_RIGHT_CLICK_CONTEXT.md around lines 19 - 22, The four consecutive bullet questions ("Can OpenCode be reused for most of the LLM/chat/interface complexity?", "Can xNet primitives own the fast local state and timeline UX?", "Can right-clicking the live UI pass useful context into OpenCode with minimal glue code?", "Are there better tools than OpenCode for this repo and this UX?") are repetitive; vary the sentence openings to improve flow by turning some questions into statements or rephrasing their leads (e.g. start one with "Reuse OpenCode for…" or "Investigate whether xNet primitives…" or "Explore if right-clicking the live UI can…" and keep the final item as a comparative question), update those four bullets accordingly so they no longer all begin with "Can" while preserving their original meaning and intent.apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx (2)
77-160: Please add one Electron-level Playwright smoke test for this flow.These hook tests are helpful, but the feature work here is mostly renderer/preload/IPC wiring. A happy-path Electron test for create/select/remount would catch regressions this suite can't see.
Based on learnings, "Test UI changes in Electron with Playwright after implementing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx` around lines 77 - 160, Add an Electron-level Playwright smoke test that exercises the same create/select/remount flow covered by the renderer hook tests: launch the app, use the renderer UI or IPC to call createSessionSummary (or trigger the UI that creates a session), verify the created session shows up and becomes active, then select the other session (or call selectSession), verify active changes, quit and relaunch the app using a persistent storage adapter (analogous to MemoryNodeStorageAdapter) and assert the session list and active selection are restored; locate existing helpers like renderWorkspaceHooks, waitForWorkspaceReady, createSessionSummary, selectSession and MemoryNodeStorageAdapter in the test suite to mirror the flow and reuse their semantics for assertions so the Playwright test validates preload/IPC wiring end-to-end.
6-12: Reorder these imports to match the repo rule.
reactandvitestare external packages, so they should stay above the internal@xnetjs/*imports.
↕️ Proposed reorderimport type { SessionSummaryNode } from '../state/active-session' import { renderHook, act, waitFor } from '@testing-library/react' +import React, { type ReactNode, useMemo } from 'react' +import { describe, expect, it } from 'vitest' import { MemoryNodeStorageAdapter } from '@xnetjs/data' import { generateIdentity } from '@xnetjs/identity' import { XNetProvider } from '@xnetjs/react' import { ConsentManager, TelemetryCollector, TelemetryProvider } from '@xnetjs/telemetry' -import React, { type ReactNode, useMemo } from 'react' -import { describe, expect, it } from 'vitest' import { useActiveSession } from './useActiveSession' import { useSessionCommands } from './useSessionCommands' import { useSessionSummaries } from './useSessionSummaries'As per coding guidelines, "
**/*.{ts,tsx}: Type-only imports must come first ... Internal package imports should follow external packages ... Local relative imports should come last`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx` around lines 6 - 12, Reorder the import block so external packages come first and internal `@xnetjs/`* imports follow: move the React import (React, type ReactNode, useMemo) and the vitest imports (describe, expect, it) above the `@xnetjs/`* imports; keep testing-library/react (renderHook, act, waitFor) and other true external libs grouped with React/vitest, then place MemoryNodeStorageAdapter, generateIdentity, XNetProvider, ConsentManager, TelemetryCollector, TelemetryProvider after them, and leave any local relative imports last; ensure import grouping follows the repo rule and preserves existing imported symbols.apps/web/src/components/PreviewContextBridge.tsx (2)
26-74: Please add one embedded right-click integration test.This path depends on real iframe/host wiring, so a unit test here won't catch the important regressions.
Based on learnings, "Test UI changes in Electron with Playwright after implementing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/PreviewContextBridge.tsx` around lines 26 - 74, Add an embedded integration test that loads the PreviewContextBridge inside a real iframe/host pair and verifies posting of PREVIEW_SELECTED_CONTEXT_MESSAGE_TYPE on right-click: write a Playwright (or Electron+Playwright) test that mounts the component (using PreviewContextBridge with a known routeId) inside an iframe, injects DOM elements with data-xnet-* attributes (used by readAttribute and buildNearbyText), simulates a contextmenu event on a target element, and asserts the parent frame receives a postMessage with routeId, targetId, targetLabel, fileHint, documentId, bounds, and nearbyText matching the element and its bounding rect; ensure the test covers fallback to the passed routeId when data-xnet-route-id is absent and cleans up the iframe after each run.
7-7: Extract this message type into a shared protocol constant.This literal is already defined in
apps/electron/src/shared/workspace-session.ts, so a future rename can silently break the bridge. Moving it into a shared web-safe module would keep both ends locked together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/PreviewContextBridge.tsx` at line 7, Replace the hard-coded PREVIEW_SELECTED_CONTEXT_MESSAGE_TYPE literal in PreviewContextBridge with a shared protocol constant: create or use an existing shared web-safe module that exports the message-type constant (the same identifier currently duplicated in the workspace session implementation), export that constant from the shared module, and import it into the PreviewContextBridge (and the workspace-session code that currently defines the duplicate) so both sides reference the single shared symbol PREVIEW_SELECTED_CONTEXT_MESSAGE_TYPE.apps/electron/electron.vite.config.ts (1)
25-28: Centralize the@xnetjs/plugins/nodealias target.This replacement path is now duplicated here and in
apps/electron/vitest.config.tsat Line 27. If the plugin entry moves, runtime and test resolution can drift independently. A small shared helper/constant for the alias target would keep both configs in sync.Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/electron.vite.config.ts` around lines 25 - 28, The alias target for xnetPluginNodeAlias is duplicated; extract the replacement path into a shared constant (e.g., XNET_PLUGIN_NODE_PATH) in a shared module and import that constant into both electron.vite.config.ts and vitest.config.ts, then set xnetPluginNodeAlias.replacement = XNET_PLUGIN_NODE_PATH (or use the constant directly when constructing the alias) so both runtime and test configs resolve the same module and stay in sync if the plugin entry moves.apps/electron/src/renderer/workspace/state/active-session.test.ts (1)
5-16: Cover the full undefined-clearing surface in this regression test.
createSessionSummaryPatchpreserves explicitundefinedforlastMessagePreviewandmodelIdtoo, so this only locks in part of the behavior added inapps/electron/src/renderer/workspace/state/active-session.ts. A single table-driven assertion over allhas(...)-guarded optionals would make this harder to regress.🧪 Suggested expansion
describe('active-session', () => { it('clears optional fields when snapshot patches unset them', () => { expect( createSessionSummaryPatch({ previewUrl: undefined, + lastMessagePreview: undefined, lastError: undefined, - lastScreenshotPath: undefined + lastScreenshotPath: undefined, + modelId: undefined }) ).toEqual({ previewUrl: undefined, + lastMessagePreview: undefined, lastError: undefined, - lastScreenshotPath: undefined + lastScreenshotPath: undefined, + modelId: undefined }) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/state/active-session.test.ts` around lines 5 - 16, The test only asserts that previewUrl, lastError, and lastScreenshotPath preserve explicit undefineds; extend the test for createSessionSummaryPatch to also include lastMessagePreview and modelId (the other optionals guarded by has(...) in active-session.ts) and convert to a table-driven assertion that iterates over the full set of optional fields (previewUrl, lastError, lastScreenshotPath, lastMessagePreview, modelId) to assert each preserves undefined when passed undefined; update the test case name accordingly and use the same call to createSessionSummaryPatch so the single assertion covers all has(...)‑guarded optionals.apps/electron/src/renderer/workspace/performance.test.ts (1)
15-29: Add one Electron Playwright smoke test for the wiring around these helpers.These unit tests prove the math, but the risk in this PR is the renderer/main integration path that triggers the marks during session switching and preview restore. A single Playwright smoke test would catch regressions that this suite cannot.
Based on learnings, "Test UI changes in Electron with Playwright after implementing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/performance.test.ts` around lines 15 - 29, Add an Electron Playwright smoke test that exercises the renderer↔main wiring for the workspace timing helpers: simulate a session switch and a preview restore in the running Electron app and verify that markWorkspaceSessionSelection, markWorkspacePreviewRestore produce the expected observable behavior by calling consumeWorkspaceSessionSelectionDuration and consumeWorkspacePreviewRestoreDuration (using the same IDs and timestamps as in the unit tests) from the test page; ensure the test opens the renderer UI, triggers the UI actions that cause the marks to be set (or directly invokes the renderer API), waits for any IPC round trips, and asserts the durations returned from the main process match expected values so integration regressions are caught.docs/plans/plan03_9_83LLMCodingUI/06-hardening-and-performance-validation.md (1)
179-189: Trim machine-specific artifacts from the committed validation notes.The absolute binary paths and local CDP endpoint here are host-specific, so they will age poorly and make this rollout doc less reusable for the next person. Summarize the outcome, but keep exact local paths in scratch notes instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/plan03_9_83LLMCodingUI/06-hardening-and-performance-validation.md` around lines 179 - 189, The validation notes contain machine-specific artifacts (absolute paths and local CDP endpoint) in rows like "OpenCode binary availability", "Electron live startup", and "Git / gh / pnpm availability"; remove or redact those exact local paths/URLs and replace them with brief, environment-agnostic summaries (e.g., "OpenCode resolved from user environment", "CDP exposed on local debug websocket") and add a short parenthetical saying "full host-specific details moved to scratch notes" so readers know where to find exact values; keep the pass/fail results and the explanatory caveats (e.g., "Pass with caveats", "Blocked by environment") intact and move the removed absolute paths/URLs into the project scratch notes or internal log instead of the committed doc.apps/electron/src/types/xnetjs-plugins-node.d.ts (1)
1-39: Replace internal source paths with the stable@xnetjs/plugins/nodeAPI.The ambient module currently depends on
packages/plugins/src/services/*internal paths. The package already exposes a stable public entrypoint at@xnetjs/plugins/node(defined inpackages/plugins/package.jsonexports, mapped todist/services/node.d.ts) which re-exports all these symbols. Using the public API instead will eliminate brittleness from Electron typings against internal refactors.Suggested fix
declare module '@xnetjs/plugins/node' { export * from '@xnetjs/plugins/node' }Or simply remove this ambient declaration entirely and let TypeScript resolve types from the installed
@xnetjs/pluginspackage exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/types/xnetjs-plugins-node.d.ts` around lines 1 - 39, The ambient module currently re-exports internal source paths (LocalAPIServer, createLocalAPI, MCPServer, ProcessManager, SERVICE_IPC_CHANNELS, etc.) which is brittle; replace the whole declaration by using the package's public API instead — either delete this ambient declaration so TypeScript uses the installed `@xnetjs/plugins` package exports, or change its body to a single public re-export (e.g. export * from '@xnetjs/plugins/node') so symbols like LocalAPIServer, createMCPServer, ProcessManager and the various type names resolve from the stable entrypoint.apps/electron/src/main/service-ipc.ts (1)
7-14: Type-only imports should be separated and placed first.As per coding guidelines, type-only imports must come before value imports. Currently the
typekeywords are inline with value imports.♻️ Proposed fix
-import { - ProcessManager, - SERVICE_IPC_CHANNELS, - type ServiceDefinition, - type ServiceOutputEvent, - type ServiceStatus, - type ServiceStatusEvent -} from '@xnetjs/plugins/node' +import type { + ServiceDefinition, + ServiceOutputEvent, + ServiceStatus, + ServiceStatusEvent +} from '@xnetjs/plugins/node' +import { ProcessManager, SERVICE_IPC_CHANNELS } from '@xnetjs/plugins/node'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/service-ipc.ts` around lines 7 - 14, Reorder the import statements so all type-only imports use the `import type { ... } from '@xnetjs/plugins/node'` form and appear before any value imports; specifically extract ServiceDefinition, ServiceOutputEvent, ServiceStatus, and ServiceStatusEvent into a separate `import type` statement placed above the value import that brings in ProcessManager and SERVICE_IPC_CHANNELS, keeping the same module and symbol names.apps/electron/src/renderer/workspace/SessionRail.test.tsx (1)
92-113: Consider adding tests for loading, error, and remove action states.The component has distinct UI states for
loadinganderrorprops, plus a Remove button for active sessions, but these aren't exercised in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/SessionRail.test.tsx` around lines 92 - 113, Add unit tests in SessionRail.test.tsx that exercise the component's loading, error, and remove-action UIs: render <SessionRail> with loading={true} and assert the loading indicator is shown; render with error={new Error('...')} and assert the error message/element is shown; render with an activeSession/activeSessionId and a mock onRemoveSession, find and click the Remove button, and assert the mock was called. Reference the SessionRail component, the loading and error props, activeSession/activeSessionId, and the onRemoveSession handler when implementing these tests.apps/electron/src/renderer/workspace/hooks/usePreviewSelectedContext.ts (2)
66-80: Potential stale closure withactiveSessionin async chain.The
activeSessionreference captured at line 66 may become stale by the time the.then()callback executes. If the user switches sessions rapidly, the state update at line 71 and theupdateSessionSummarycall at line 74 could operate on an outdated session.Consider capturing
activeSession.idin a local variable before the async chain, or using a ref to track the current active session ID and bail out if it changes.Suggested approach
+ const targetSessionId = activeSession.id + void storeWorkspaceSelectedContext(activeSession, nextContext) .then((stored) => { + // Bail out if session changed during async operation + if (targetSessionId !== activeSession?.id) { + return + } const prompt = buildSelectedContextPrompt(nextContext, stored.path) setContextsBySessionId((current) => ({ ...current, - [activeSession.id]: nextContext + [targetSessionId]: nextContext }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/usePreviewSelectedContext.ts` around lines 66 - 80, The async chain captures activeSession directly which can become stale; before calling storeWorkspaceSelectedContext, copy the session id into a local const (e.g. const sessionId = activeSession.id) and use that sessionId inside the .then() callbacks for setContextsBySessionId, buildSelectedContextPrompt, updateSessionSummary and window.xnetOpenCode.appendPrompt; alternatively read a currentSessionId ref/state at the start of the chain and bail inside .then() if it no longer matches to avoid applying updates to the wrong session (affecting storeWorkspaceSelectedContext, buildSelectedContextPrompt, setContextsBySessionId, updateSessionSummary).
78-80: Error handling only logs; consider surfacing failures to the user.When
storeWorkspaceSelectedContextfails, the error is logged but the user receives no feedback. For a coding workspace where context capture is user-initiated, silent failures may cause confusion.Consider reporting the error via telemetry or displaying a toast notification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/usePreviewSelectedContext.ts` around lines 78 - 80, The catch block in usePreviewSelectedContext that currently logs errors from storeWorkspaceSelectedContext should also surface failures to the user and telemetry: update the .catch((error) => { ... }) handler to call the app's telemetry/reporting API (e.g., sendTelemetryEvent or reportError) with the error and context, and trigger a user-facing notification/toast (e.g., showToast or notifyUser) describing the failure and suggested next steps; keep the console.error for debugging but ensure both telemetry and a concise toast are emitted when storeWorkspaceSelectedContext fails.apps/electron/src/main/preview-manager.ts (2)
419-427: Port allocation based onruntimes.sizemay cause collisions after session removal.When sessions are removed,
runtimes.sizedecreases, but previously allocated ports aren't released. The nextallocatePortcall could compute anextPortthat was already assigned to an active runtime.For example: 3 sessions use ports 4310, 4311, 4312. Session 2 is removed.
runtimes.sizeis now 2. New session callsallocatePort, getsbasePort + 2 = 4312, which collides with session 3.The
findAvailablePortcall will skip occupied ports, but this is inefficient. Consider tracking the high-water mark of allocated ports instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/preview-manager.ts` around lines 419 - 427, The current allocatePort logic uses this.runtimes.size to compute nextPort which can collide after removals; change it to maintain a high-water mark counter (e.g., this.nextPortCounter or this.maxAllocatedPort) that is initialized from basePort and incremented when allocatePort assigns a port, and update it only on allocation (not on removal), then pass that incremented value to findAvailablePort(this.host, nextPort); also ensure the runtime removal path does not decrement the counter so active ports stay unique. Reference: allocatePort, runtimes, basePort, findAvailablePort.
53-60: DuplicatefileExistshelper.This exact implementation is duplicated in
apps/electron/src/main/workspace-session-ipc.ts(lines 43-50). Consider extracting to a shared utility module to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/preview-manager.ts` around lines 53 - 60, Duplicate helper function fileExists is implemented in multiple modules; extract it into a single shared utility (e.g., export const fileExists in a common utils module) and update existing callers (those currently declaring fileExists) to import and use the shared function instead of redeclaring it; ensure the exported function preserves the same signature Promise<boolean> and behavior (uses access with constants.F_OK) and update imports in the modules that previously declared fileExists to remove the local definition and import the shared utility.apps/electron/src/renderer/workspace/SessionRail.tsx (1)
89-130: Logic issue: "Sessions" group returned at lines 117-127 discardsnodesarray.When
activeSessionIdisnull, the logic at lines 107-115 may push an "Other Sessions" group tonodes, but then lines 117-127 return a completely new array, discarding any content previously pushed tonodes.Additionally, when
activeSessionIdis null and there are sessions, users would see both the "Sessions" group (from line 120) containing all sessions, and potentially duplicate content if the early-return weren't present.The current flow appears to work because of the guard conditions, but the structure is confusing. Consider simplifying:
Suggested refactor
const treeNodes = useMemo(() => { if (sessions.length === 0) { return [] } - const [selected, ...rest] = sessions - const nodes: TreeNode[] = [] - - if (activeSessionId && selected && selected.id === activeSessionId) { - nodes.push({ - id: 'active-session-group', - label: 'Active Session', - defaultExpanded: true, - badge: '1', - children: [createSessionNode(selected, onSelectSession)] - }) - } - - if (rest.length > 0) { - nodes.push({ - id: 'other-sessions-group', - label: activeSessionId ? 'Other Sessions' : 'Sessions', - defaultExpanded: true, - badge: String(rest.length), - children: rest.map((session) => createSessionNode(session, onSelectSession)) - }) - } - - if (!activeSessionId) { - return [ - { - id: 'sessions-group', - label: 'Sessions', - defaultExpanded: true, - badge: String(sessions.length), - children: sessions.map((session) => createSessionNode(session, onSelectSession)) - } - ] - } - - return nodes + if (!activeSessionId) { + return [ + { + id: 'sessions-group', + label: 'Sessions', + defaultExpanded: true, + badge: String(sessions.length), + children: sessions.map((session) => createSessionNode(session, onSelectSession)) + } + ] + } + + const activeSession = sessions.find((s) => s.id === activeSessionId) + const otherSessions = sessions.filter((s) => s.id !== activeSessionId) + const nodes: TreeNode[] = [] + + if (activeSession) { + nodes.push({ + id: 'active-session-group', + label: 'Active Session', + defaultExpanded: true, + badge: '1', + children: [createSessionNode(activeSession, onSelectSession)] + }) + } + + if (otherSessions.length > 0) { + nodes.push({ + id: 'other-sessions-group', + label: 'Other Sessions', + defaultExpanded: true, + badge: String(otherSessions.length), + children: otherSessions.map((session) => createSessionNode(session, onSelectSession)) + }) + } + + return nodes }, [activeSessionId, onSelectSession, sessions])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/SessionRail.tsx` around lines 89 - 130, The useMemo that builds treeNodes sometimes pushes groups into the nodes array and then unconditionally returns a new "Sessions" group when activeSessionId is falsy, which discards earlier work and is confusing; change the control flow in the treeNodes useMemo so you either (A) early-return the full "Sessions" group before you build any nodes when activeSessionId is null, or (B) always build into the nodes array and return nodes (without creating a separate sessions-group), updating the logic around activeSessionId, selected, rest and the creation calls to createSessionNode to avoid duplicates—i.e., check activeSessionId first, return the single sessions-group if null, else proceed to build nodes with active-session-group and other-sessions-group and return nodes.apps/electron/src/main/workspace-session-ipc.ts (1)
43-50: DuplicatefileExistshelper.This is identical to the implementation in
apps/electron/src/main/preview-manager.ts(lines 53-60). Extract to a shared utility module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/workspace-session-ipc.ts` around lines 43 - 50, The duplicate async helper fileExists should be extracted to a shared utility: create a new exported function (e.g., export const fileExists = async (path: string): Promise<boolean> => { ... }) in a common utils module and replace the duplicate implementations in workspace-session-ipc and preview-manager by importing that shared fileExists; update both files to remove the local fileExists definitions and import the new utility, keeping the same behavior (uses access(path, constants.F_OK) and returns true/false) and export name so other callers don’t need changes.apps/electron/src/renderer/workspace/WorkspaceStateBootstrap.tsx (1)
13-19: Consider surfacing initialization errors.The
voidcast on Line 18 discards any rejected promise. WhileensureWorkspaceShellStatelikely handles errors internally via telemetry (based on themeasureCommandwrapper inuseSessionCommands), failed initialization could leave the workspace in an inconsistent state without user visibility.If initialization failures should be actionable, consider tracking error state and surfacing recovery options. Otherwise, this fire-and-forget pattern is acceptable for non-critical bootstrap logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/WorkspaceStateBootstrap.tsx` around lines 13 - 19, The current useEffect fire-and-forget call to ensureWorkspaceShellState() (wrapped in a void) swallows rejections — change the effect to await the promise and handle errors: call ensureWorkspaceShellState() inside an async function in the effect, catch errors and set a new local state like initError (e.g., via useState) or invoke an existing error handler so the UI can surface retry/recovery actions; if you still want telemetry, rethrow or forward the caught error to the existing measureCommand/error-reporting path (referencing ensureWorkspaceShellState and the useEffect conditions using loading and shellState) so failures are not silently discarded.apps/electron/src/renderer/workspace/panels/ChangedFilesPanel.tsx (1)
5-7: Import order does not follow coding guidelines.External package imports (
react) should come before internal package imports (@xnetjs/ui). As per coding guidelines: "External package imports should follow type-only imports" and "Internal package imports should follow external packages."🔧 Suggested fix
import type { WorkspaceSessionReview } from '../../../shared/workspace-session' -import { Badge } from '@xnetjs/ui' import React from 'react' +import { Badge } from '@xnetjs/ui'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/panels/ChangedFilesPanel.tsx` around lines 5 - 7, Reorder the imports so they follow the project guidelines: keep the type-only import (WorkspaceSessionReview) first, then external package imports (React), and then internal package imports (Badge from `@xnetjs/ui`); update the import order in ChangedFilesPanel.tsx accordingly so it reads type imports → external packages → internal packages (reference the existing symbols WorkspaceSessionReview, React, and Badge to locate the lines to change).apps/electron/src/renderer/workspace/hooks/useActiveSession.ts (3)
14-24: Add explicit return type to exported function.Per coding guidelines: "Use explicit return types on exported functions."
🔧 Suggested fix
-export function useActiveSessionId() { +export function useActiveSessionId(): { + shellState: ReturnType<typeof useQuery<typeof WorkspaceShellStateSchema, string>>['data'] + activeSessionId: string | null + loading: boolean + error: Error | null + reload: () => Promise<void> +} { const query = useQuery(WorkspaceShellStateSchema, WORKSPACE_SHELL_STATE_NODE_ID)Alternatively, extract a named type for the return value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useActiveSession.ts` around lines 14 - 24, Add an explicit return type to the exported function useActiveSessionId: define a named type or inline type describing { shellState: ReturnType<typeof useQuery>["data"], activeSessionId: string | null, loading: boolean, error: unknown, reload: () => Promise<void> } (or the precise types from WorkspaceShellStateSchema) and annotate the function signature (export function useActiveSessionId(): YourReturnType) so the exported symbol has an explicit type; reference the useQuery call and the query variable to ensure the types for shellState, activeSessionId, loading, error, and reload match the actual values.
5-6: Import order does not follow coding guidelines.External package imports (
react) should come before internal package imports (@xnetjs/react).🔧 Suggested fix
-import { useQuery } from '@xnetjs/react' import { useCallback, useMemo } from 'react' +import { useQuery } from '@xnetjs/react' import { SessionSummarySchema, WorkspaceShellStateSchema } from '../schemas'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useActiveSession.ts` around lines 5 - 6, The import order in useActiveSession.ts is reversed: move the external React imports before internal packages. Specifically, place "import { useCallback, useMemo } from 'react'" above "import { useQuery } from '@xnetjs/react'"; ensure the file uses the same named symbols (useCallback, useMemo, useQuery) and that linting/import-order rules are satisfied after the change.
46-48: Dependency array uses unstable object references.The
reloadcallback depends on the entiresessionSummaryQueryandshellStateQueryobjects, which are recreated on every render. This causes the callback to be unnecessarily recreated, potentially triggering downstream effects.🔧 Suggested fix
const reload = useCallback(async () => { await Promise.all([shellStateQuery.reload(), sessionSummaryQuery.reload()]) - }, [sessionSummaryQuery, shellStateQuery]) + }, [sessionSummaryQuery.reload, shellStateQuery.reload])The
.reloadmethods are memoized byuseQueryand provide stable references, so depending on them directly avoids unnecessary callback recreation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useActiveSession.ts` around lines 46 - 48, The useCallback for reload currently lists the whole sessionSummaryQuery and shellStateQuery objects in its dependency array, causing unnecessary recreations; update the dependency array to reference the stable .reload methods instead (i.e., use sessionSummaryQuery.reload and shellStateQuery.reload) so the callback depends only on those memoized functions and not on the enclosing query objects in the useCallback that defines reload.apps/electron/src/renderer/workspace/hooks/useSessionSummaries.ts (1)
5-6: Import order does not follow coding guidelines.External package imports (
react) should come before internal package imports (@xnetjs/react).🔧 Suggested fix
-import { useQuery } from '@xnetjs/react' import { useCallback, useMemo } from 'react' +import { useQuery } from '@xnetjs/react' import { SessionSummarySchema } from '../schemas'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useSessionSummaries.ts` around lines 5 - 6, Reorder the import statements so external packages come before internal ones: move "import { useCallback, useMemo } from 'react'" above "import { useQuery } from '@xnetjs/react'"; update the top of apps/electron/src/renderer/workspace/hooks/useSessionSummaries.ts accordingly to follow the project's import ordering guideline.apps/electron/src/renderer/workspace/hooks/useWorkspaceReview.ts (1)
5-9: Import order does not follow coding guidelines.External package imports (
react) should come before internal package imports (@xnetjs/telemetry).🔧 Suggested fix
import type { WorkspaceSessionReview } from '../../../shared/workspace-session' import type { SessionSummaryNode } from '../state/active-session' -import { useTelemetry } from '@xnetjs/telemetry' import { useCallback, useEffect, useState } from 'react' +import { useTelemetry } from '@xnetjs/telemetry' import { useSessionCommands } from './useSessionCommands'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceReview.ts` around lines 5 - 9, Reorder the imports in useWorkspaceReview.ts so external packages come first: move "react" imports (useCallback, useEffect, useState) above the internal package import "useTelemetry" from '@xnetjs/telemetry', keeping type imports (WorkspaceSessionReview, SessionSummaryNode) and local hooks (useSessionCommands) after package imports; update the import order in the file to follow the project's import ordering guideline.apps/electron/src/main/opencode-service.ts (1)
135-140: Fix forEach callbacks to not return values.Static analysis flags that these
forEachcallbacks return the result ofSet.add(), which is a code smell even thoughforEachignores return values.🔧 Suggested fix
const pathCandidates = getPathCandidates() - pathCandidates.forEach((candidate) => uniqueCandidates.add(candidate)) + for (const candidate of pathCandidates) { + uniqueCandidates.add(candidate) + } const commonCandidates = getCommonBinaryLocations() - commonCandidates.forEach((candidate) => uniqueCandidates.add(candidate)) + for (const candidate of commonCandidates) { + uniqueCandidates.add(candidate) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/opencode-service.ts` around lines 135 - 140, The forEach callbacks currently return the result of uniqueCandidates.add(candidate) which is unnecessary; update the callbacks that iterate over getPathCandidates() and getCommonBinaryLocations() to use statement bodies that call uniqueCandidates.add(candidate) without returning it (e.g., use { uniqueCandidates.add(candidate); } or a for...of loop) so the callback does not return a value; reference the calls that invoke uniqueCandidates.add inside the forEach surrounding getPathCandidates and getCommonBinaryLocations.apps/electron/src/renderer/workspace/DevWorkspaceShell.tsx (2)
5-16: Import order does not follow coding guidelines.External package imports (
lucide-react,react) should come before internal package imports (@xnetjs/telemetry,@xnetjs/ui).🔧 Suggested fix
-import { useTelemetry } from '@xnetjs/telemetry' -import { Badge, Button, ResizableHandle, ResizablePanel, ResizablePanelGroup } from '@xnetjs/ui' import { ArrowLeft, Code2, RefreshCcw, Settings, Trash2 } from 'lucide-react' import React, { useCallback, useEffect, useState } from 'react' +import { useTelemetry } from '@xnetjs/telemetry' +import { Badge, Button, ResizableHandle, ResizablePanel, ResizablePanelGroup } from '@xnetjs/ui' import { useActiveSession } from './hooks/useActiveSession'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/DevWorkspaceShell.tsx` around lines 5 - 16, The import block in DevWorkspaceShell.tsx is out of order; move external package imports (e.g., "react" and "lucide-react" — symbols like React, ArrowLeft, Code2, RefreshCcw, Settings, Trash2) above internal package imports (e.g., "@xnetjs/telemetry" and "@xnetjs/ui" — symbols like useTelemetry, Badge, Button, ResizableHandle, ResizablePanel, ResizablePanelGroup) and keep local project hooks/components (useActiveSession, usePreviewSelectedContext, useSessionCommands, useWorkspaceSessionSync, OpenCodePanel, PreviewWorkspace, SessionRail, consumeWorkspaceSessionDuration) after them so the file follows the repository's import ordering guidelines.
156-276: Consider adding Playwright tests for the workspace shell UI.The three-panel layout with resizable panels, session management actions, and notice rendering involves significant UI interactions that would benefit from end-to-end testing. Based on learnings: "Test UI changes in Electron with Playwright after implementing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/DevWorkspaceShell.tsx` around lines 156 - 276, Add Playwright end-to-end tests exercising the DevWorkspaceShell component: create tests that launch the Electron renderer mounting DevWorkspaceShell and interact with ResizablePanelGroup (drag handles to resize), SessionRail (click create via handleCreateSession, select sessions via onSelectSession, and remove via handleRemoveActiveSession), the action buttons (Refresh -> handleRefresh, Remove, Settings, Canvas), and PreviewWorkspace refresh/restart handlers; also assert shellNotice renders correct tone/message when provided and that summaries length badge updates. Locate test targets by component names DevWorkspaceShell, SessionRail, OpenCodePanel, PreviewWorkspace, ResizablePanelGroup and handler names handleCreateSession/handleRefresh/handleRemoveActiveSession/handleSelectSession/handleRestartPreview, and implement assertions for layout changes, session count text, selected session title/branch, and notice styling/contents. Ensure tests run in Electron Playwright environment and cover resize, button clicks, and conditional rendering flows.apps/electron/src/preload/index.ts (1)
569-601: Tighten the preload API types while you're here.The runtime allowlist is a good hardening step, but the public service bridge still accepts raw
stringchannels, and the new OpenCode/workspace bridge shapes are modeled asinterfaces. UsingXNetServiceChannelin the signatures andtypealiases for the new object shapes keeps the public contract aligned with the runtime guard and repo conventions. As per coding guidelines, "Prefertypeoverinterfacefor defining object shapes in TypeScript".💡 Suggested fix
-export interface XNetServicesAPI { - invoke<T>(channel: string, ...args: unknown[]): Promise<T> - on(channel: string, handler: (...args: unknown[]) => void): void - off(channel: string, handler: (...args: unknown[]) => void): void -} +export type XNetServicesAPI = { + invoke<T>(channel: XNetServiceChannel, ...args: unknown[]): Promise<T> + on(channel: XNetServiceChannel, handler: (...args: unknown[]) => void): void + off(channel: XNetServiceChannel, handler: (...args: unknown[]) => void): void +}Apply the same
type-alias shape toXNetOpenCodeAPIandXNetWorkspaceSessionsAPI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/preload/index.ts` around lines 569 - 601, Replace loose string channels and interfaces with the tighter type aliases: update XNetServicesAPI to use XNetServiceChannel instead of `channel: string` in invoke/on/off, and convert the XNetOpenCodeAPI and XNetWorkspaceSessionsAPI interface declarations into type aliases (type XNetOpenCodeAPI = { ... } and type XNetWorkspaceSessionsAPI = { ... }) so their shapes follow the repo convention and align with the runtime allowlist represented by XNetServiceChannel.docs/plans/plan03_9_83LLMCodingUI/02-session-summary-and-fast-shell-state.md (1)
197-205: Add a Playwright step to this validation section.This shell-state work crosses main, preload, and renderer boundaries, so hook tests plus manual checks will miss the integration regressions that matter most in Electron. Based on learnings, Test UI changes in Electron with Playwright after implementing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/plan03_9_83LLMCodingUI/02-session-summary-and-fast-shell-state.md` around lines 197 - 205, Update the "Testing and Validation Approach" section to add a Playwright-based integration step: describe running a Playwright test that launches the Electron app, navigates to the shell UI, seeds or selects session summaries, verifies the rail renders summaries from local state, toggles the active session offline and confirms the shell/shell state still renders correctly across main, preload, and renderer boundaries; include notes on running the Playwright test in CI and any required test fixtures or launch flags for Electron so integrational regressions are caught beyond hook-level and manual checks.apps/electron/src/renderer/workspace/hooks/useSessionCommands.ts (1)
260-267: Consider partial failure handling in batch updates.
Promise.allwill reject if any singleupdateSessionSummaryfails, potentially losing successful updates. If partial success is acceptable, considerPromise.allSettledwith error logging for failed updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useSessionCommands.ts` around lines 260 - 267, The batch update uses Promise.all which rejects on the first failure, risking loss of other successful updates; replace the Promise.all call around snapshots.map(...) with Promise.allSettled, then iterate the settled results and for any result.status === "rejected" call your logger (or console.error) including the corresponding snapshot.sessionId and the error; keep successful results as-is so partial success is preserved and reference the functions updateSessionSummary and createSessionSummaryPatchFromWorkspaceSnapshot and the snapshots array when implementing the change.apps/electron/src/main/opencode-host-controller.ts (1)
123-127: Consider handling 'stopping' state in ensure().When
current.stateis'stopping', the function returns early viarefresh(), which may report a transient status. Consider waiting for the service to fully stop before potentially restarting, or explicitly checking for'stopping'state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/opencode-host-controller.ts` around lines 123 - 127, The ensure() function currently treats any non-'error' state as eligible for refresh; update ensure() to explicitly handle the 'stopping' state returned by deps.getServiceStatus(OPENCODE_SERVICE_ID) instead of immediately calling refresh(): if current.state === 'stopping' poll/wait (e.g., loop with short delay) until the service transitions to 'stopped' or 'error' (or a timeout) and then proceed to refresh or restart; ensure you reference the existing ensure, deps.getServiceStatus and refresh symbols when adding the check and waiting logic and surface a clear outcome if the stop times out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 232e5982-0057-4aec-b542-84fc8d944d3a
📒 Files selected for processing (68)
apps/electron/README.mdapps/electron/electron.vite.config.tsapps/electron/src/__tests__/opencode-host-controller.test.tsapps/electron/src/__tests__/service-ipc.test.tsapps/electron/src/main/command-errors.test.tsapps/electron/src/main/command-errors.tsapps/electron/src/main/git-service.test.tsapps/electron/src/main/git-service.tsapps/electron/src/main/index.tsapps/electron/src/main/opencode-host-controller.tsapps/electron/src/main/opencode-service.tsapps/electron/src/main/preview-manager.test.tsapps/electron/src/main/preview-manager.tsapps/electron/src/main/service-ipc.tsapps/electron/src/main/workspace-session-ipc.tsapps/electron/src/preload/index.tsapps/electron/src/renderer/App.tsxapps/electron/src/renderer/components/SystemMenu.tsxapps/electron/src/renderer/index.htmlapps/electron/src/renderer/main.tsxapps/electron/src/renderer/workspace/DevWorkspaceShell.tsxapps/electron/src/renderer/workspace/OpenCodePanel.tsxapps/electron/src/renderer/workspace/PreviewWorkspace.tsxapps/electron/src/renderer/workspace/SessionRail.test.tsxapps/electron/src/renderer/workspace/SessionRail.tsxapps/electron/src/renderer/workspace/WorkspaceStateBootstrap.tsxapps/electron/src/renderer/workspace/context/selected-context.test.tsapps/electron/src/renderer/workspace/context/selected-context.tsapps/electron/src/renderer/workspace/hooks/useActiveSession.tsapps/electron/src/renderer/workspace/hooks/usePreviewSelectedContext.tsapps/electron/src/renderer/workspace/hooks/useSessionCommands.tsapps/electron/src/renderer/workspace/hooks/useSessionSummaries.tsapps/electron/src/renderer/workspace/hooks/useWorkspaceReview.tsapps/electron/src/renderer/workspace/hooks/useWorkspaceSessionSync.tsapps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsxapps/electron/src/renderer/workspace/panels/ChangedFilesPanel.tsxapps/electron/src/renderer/workspace/panels/DiffPanel.tsxapps/electron/src/renderer/workspace/panels/MarkdownPreviewPanel.tsxapps/electron/src/renderer/workspace/panels/PrDraftPanel.tsxapps/electron/src/renderer/workspace/performance.test.tsapps/electron/src/renderer/workspace/performance.tsapps/electron/src/renderer/workspace/schemas.tsapps/electron/src/renderer/workspace/state/active-session.test.tsapps/electron/src/renderer/workspace/state/active-session.tsapps/electron/src/shared/opencode-host.tsapps/electron/src/shared/service-ipc.tsapps/electron/src/shared/workspace-session.tsapps/electron/src/types/xnetjs-plugins-node.d.tsapps/electron/tsconfig.node.jsonapps/electron/tsconfig.web.jsonapps/electron/vitest.config.tsapps/web/src/components/DatabaseView.tsxapps/web/src/components/PreviewContextBridge.tsxapps/web/src/routes/__root.tsxapps/web/src/routes/canvas.$canvasId.tsxapps/web/src/routes/db.$dbId.tsxdocs/explorations/0107_[_]_SELF_EDITING_APP_IN_APP_DEVELOPMENT_GIT_PR_WORKTREE_UX_VIABILITY_ACROSS_WEB_ELECTRON_MOBILE.mddocs/explorations/0108_[_]_ELECTRON_FIRST_SELF_EDITING_WORKSPACE_SHELL_CODEX_LIKE_CHATS_WORKTREES_BRANCHES_PREVIEW_DIFF_PRS_OPENCODE_INTEGRATION.mddocs/explorations/0109_[_]_ELECTRON_MVP_SELF_EDITING_WORKSPACE_CHEAP_PERFORMANCE_WINS_OPENCODE_WORKTREES_RIGHT_CLICK_CONTEXT.mddocs/plans/plan03_9_83LLMCodingUI/01-service-boundary-and-opencode-host.mddocs/plans/plan03_9_83LLMCodingUI/02-session-summary-and-fast-shell-state.mddocs/plans/plan03_9_83LLMCodingUI/03-shell-layout-and-opencode-panel.mddocs/plans/plan03_9_83LLMCodingUI/04-worktree-and-preview-orchestration.mddocs/plans/plan03_9_83LLMCodingUI/05-context-bridge-diff-pr-surface.mddocs/plans/plan03_9_83LLMCodingUI/06-hardening-and-performance-validation.mddocs/plans/plan03_9_83LLMCodingUI/README.mdpackages/views/src/board/BoardView.tsxpackages/views/src/table/TableView.tsx
| - OpenCode missing: install from [OpenCode docs](https://opencode.ai/docs/install), then refresh the center panel | ||
| - Preview startup failure: run `pnpm install`, then `pnpm run deps:electron`, then restart the preview from the right panel | ||
| - If `pnpm run deps:electron` fails inside `@electron/rebuild` with `util.styleText is not a function`, rebuild the native module directly with `npm rebuild better-sqlite3 --runtime=electron --target=33.4.11 --arch=arm64 --dist-url=https://electronjs.org/headers` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pkg="$(fd '^package\.json$' apps/electron | head -n1)"
echo "package.json: $pkg"
echo "Electron version in package.json:"
jq -r '.devDependencies.electron // .dependencies.electron // "missing"' "$pkg"
echo
echo "Hard-coded rebuild fallback in README:"
rg -n -- '--target=|--arch=' apps/electron/README.mdRepository: crs48/xNet
Length of output: 443
🏁 Script executed:
# Check if there are other version/arch-specific values in the README
echo "=== Full context around line 73 ==="
sed -n '65,80p' apps/electron/README.md
echo -e "\n=== Check for other hard-coded versions in README ==="
rg -n '(\d+\.\d+\.\d+|arm64|x64|--arch)' apps/electron/README.md | head -20
echo -e "\n=== Check package.json for electron and better-sqlite3 versions ==="
jq '.devDependencies | with_entries(select(.key | test("electron|sqlite")))' apps/electron/package.jsonRepository: crs48/xNet
Length of output: 1494
Replace hard-coded Electron target and arch with dynamic or placeholder values.
Line 73 hard-codes --target=33.4.11 and --arch=arm64 in the recovery fallback. The --arch=arm64 will fail silently or mislead developers on x64 and other architectures. The --target=33.4.11 is currently compatible with the app's configured ^33.0.0 but will drift once Electron is upgraded to 34.x or later. Extract or document these as placeholders (e.g., --arch=$(uname -m) or reference the actual Electron version from package.json).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/README.md` around lines 71 - 73, Update the README recovery
command that runs `npm rebuild better-sqlite3 --runtime=electron
--target=33.4.11 --arch=arm64` to avoid hard-coded values: replace the fixed
`--target=33.4.11` and `--arch=arm64` with dynamic/placeholder references (e.g.,
document using the project's Electron version and the system architecture) and
show how to obtain them (for example, reference reading Electron version from
package.json or using a shell substitution like uname -m for arch) so the `npm
rebuild better-sqlite3` fallback is correct across Electron upgrades and CPU
architectures.
| const task = this.resolveRepoContextInternal(startPath) | ||
| if (!startPath) { | ||
| this.repoContextPromise = task | ||
| } |
There was a problem hiding this comment.
Don't cache a rejected repo-context promise.
If the first resolveRepoContext() attempt fails, repoContextPromise stays rejected and every later call without startPath will fail forever. Clear the cache on rejection so transient Git/environment issues can recover.
💡 Suggested fix
async resolveRepoContext(startPath?: string): Promise<GitRepoContext> {
if (!startPath && this.repoContextPromise) {
return this.repoContextPromise
}
const task = this.resolveRepoContextInternal(startPath)
if (!startPath) {
- this.repoContextPromise = task
+ this.repoContextPromise = task.catch((error) => {
+ this.repoContextPromise = null
+ throw error
+ })
+ return this.repoContextPromise
}
return task
}📝 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.
| const task = this.resolveRepoContextInternal(startPath) | |
| if (!startPath) { | |
| this.repoContextPromise = task | |
| } | |
| const task = this.resolveRepoContextInternal(startPath) | |
| if (!startPath) { | |
| this.repoContextPromise = task.catch((error) => { | |
| this.repoContextPromise = null | |
| throw error | |
| }) | |
| return this.repoContextPromise | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/main/git-service.ts` around lines 244 - 247, The current
logic assigns a rejected promise to this.repoContextPromise (in
resolveRepoContext when startPath is falsy) which permanently caches failures;
change the assignment so that when you set this.repoContextPromise = task you
attach a rejection handler that clears the cache and rethrows the error (e.g.,
set this.repoContextPromise = task.catch(err => { this.repoContextPromise =
undefined; throw err; })), referencing resolveRepoContextInternal,
resolveRepoContext, repoContextPromise and startPath so transient failures don't
leave repoContextPromise rejected.
| private async runCommand( | ||
| command: string, | ||
| args: readonly string[], | ||
| cwd: string | ||
| ): Promise<CommandResult> { | ||
| try { | ||
| const result = await execFileAsync(command, [...args], { | ||
| cwd, | ||
| env: process.env, | ||
| maxBuffer: 10 * 1024 * 1024 | ||
| }) | ||
|
|
||
| return { | ||
| stdout: result.stdout ?? '', | ||
| stderr: result.stderr ?? '' | ||
| } | ||
| } catch (error) { | ||
| throw new Error(formatCommandFailure(command, args, cwd, error)) | ||
| } |
There was a problem hiding this comment.
Make these CLI calls non-interactive and bounded.
These commands sit on the main-process IPC path with no timeout. A Git credential prompt, lock wait, hook, or interactive gh pr create flow can leave the renderer waiting forever. Add a timeout and disable interactive prompting in the child env before wiring this behind user actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/main/git-service.ts` around lines 420 - 438, runCommand
currently spawns CLI tools with the ambient env and no timeout; make the child
process non-interactive and bounded by adding a timeout option to execFileAsync
and sanitizing/augmenting the child env to disable prompts. Update runCommand to
call execFileAsync with a timeout (e.g. a few seconds) and pass env: {
...process.env, GIT_TERMINAL_PROMPT: '0', GIT_ASKPASS: 'true', SSH_ASKPASS:
'true', GH_TERMINAL_PROMPT: '0' } (or equivalent non-interactive stubs) so
Git/GH won't prompt, and ensure maxBuffer remains reasonable; propagate errors
as before using formatCommandFailure(command, args, cwd, error).
| async function probeOpenCodeHealth( | ||
| config: OpenCodeHostConfig | ||
| ): Promise<OpenCodeHealthPayload | null> { | ||
| const controller = new AbortController() | ||
| const timeout = setTimeout(() => controller.abort(), 2000) | ||
|
|
||
| try { | ||
| const response = await fetch(`${config.baseUrl}${OPENCODE_HEALTH_PATH}`, { | ||
| headers: getOpenCodeAuthHeaders(config), | ||
| signal: controller.signal | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| return null | ||
| } | ||
|
|
||
| return (await response.json()) as OpenCodeHealthPayload | ||
| } catch { | ||
| return null | ||
| } finally { | ||
| clearTimeout(timeout) | ||
| } | ||
| } |
There was a problem hiding this comment.
Variable controller shadows module-level declaration.
Line 170 declares a local const controller = new AbortController() which shadows the module-level controller (the OpenCode host controller) declared at line 75. This can cause confusion and potential bugs if someone tries to use the host controller inside this function.
🔧 Suggested fix
async function probeOpenCodeHealth(
config: OpenCodeHostConfig
): Promise<OpenCodeHealthPayload | null> {
- const controller = new AbortController()
- const timeout = setTimeout(() => controller.abort(), 2000)
+ const abortController = new AbortController()
+ const timeout = setTimeout(() => abortController.abort(), 2000)
try {
const response = await fetch(`${config.baseUrl}${OPENCODE_HEALTH_PATH}`, {
headers: getOpenCodeAuthHeaders(config),
- signal: controller.signal
+ signal: abortController.signal
})📝 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.
| async function probeOpenCodeHealth( | |
| config: OpenCodeHostConfig | |
| ): Promise<OpenCodeHealthPayload | null> { | |
| const controller = new AbortController() | |
| const timeout = setTimeout(() => controller.abort(), 2000) | |
| try { | |
| const response = await fetch(`${config.baseUrl}${OPENCODE_HEALTH_PATH}`, { | |
| headers: getOpenCodeAuthHeaders(config), | |
| signal: controller.signal | |
| }) | |
| if (!response.ok) { | |
| return null | |
| } | |
| return (await response.json()) as OpenCodeHealthPayload | |
| } catch { | |
| return null | |
| } finally { | |
| clearTimeout(timeout) | |
| } | |
| } | |
| async function probeOpenCodeHealth( | |
| config: OpenCodeHostConfig | |
| ): Promise<OpenCodeHealthPayload | null> { | |
| const abortController = new AbortController() | |
| const timeout = setTimeout(() => abortController.abort(), 2000) | |
| try { | |
| const response = await fetch(`${config.baseUrl}${OPENCODE_HEALTH_PATH}`, { | |
| headers: getOpenCodeAuthHeaders(config), | |
| signal: abortController.signal | |
| }) | |
| if (!response.ok) { | |
| return null | |
| } | |
| return (await response.json()) as OpenCodeHealthPayload | |
| } catch { | |
| return null | |
| } finally { | |
| clearTimeout(timeout) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/main/opencode-service.ts` around lines 167 - 189, The local
variable controller in probeOpenCodeHealth shadows the module-level controller;
rename the local AbortController (e.g., probeController or healthController)
used inside probeOpenCodeHealth, update its references (signal and
clearTimeout), and keep the same timeout/abort behavior so the module-level
OpenCode host controller is not shadowed or confused with the local probe
controller.
| async function findAvailablePort(host: string, startPort: number): Promise<number> { | ||
| let port = startPort | ||
|
|
||
| while (!(await isPortAvailable(host, port))) { | ||
| port += 1 | ||
| } | ||
|
|
||
| return port | ||
| } |
There was a problem hiding this comment.
Unbounded port search could loop indefinitely.
If all ports are unavailable (e.g., due to ephemeral port exhaustion or firewall rules), findAvailablePort will loop forever. Consider adding an upper bound.
Suggested fix
-async function findAvailablePort(host: string, startPort: number): Promise<number> {
+async function findAvailablePort(
+ host: string,
+ startPort: number,
+ maxAttempts: number = 100
+): Promise<number> {
let port = startPort
+ let attempts = 0
- while (!(await isPortAvailable(host, port))) {
+ while (!(await isPortAvailable(host, port))) {
+ if (++attempts >= maxAttempts) {
+ throw new Error(`No available port found after ${maxAttempts} attempts starting from ${startPort}`)
+ }
port += 1
}
return port
}📝 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.
| async function findAvailablePort(host: string, startPort: number): Promise<number> { | |
| let port = startPort | |
| while (!(await isPortAvailable(host, port))) { | |
| port += 1 | |
| } | |
| return port | |
| } | |
| async function findAvailablePort( | |
| host: string, | |
| startPort: number, | |
| maxAttempts: number = 100 | |
| ): Promise<number> { | |
| let port = startPort | |
| let attempts = 0 | |
| while (!(await isPortAvailable(host, port))) { | |
| if (++attempts >= maxAttempts) { | |
| throw new Error(`No available port found after ${maxAttempts} attempts starting from ${startPort}`) | |
| } | |
| port += 1 | |
| } | |
| return port | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/main/preview-manager.ts` around lines 96 - 104, The
findAvailablePort function currently loops until isPortAvailable returns true
with no upper bound; modify findAvailablePort(host, startPort) to accept or
enforce a max port (e.g., clamp to 65535 or add an optional maxPort parameter),
validate startPort is within range, iterate from startPort up to maxPort, and if
no port is found throw a clear error (or return a rejected Promise) instead of
looping forever; update callers accordingly to handle the thrown error/Promise
rejection.
| useEffect(() => { | ||
| let active = true | ||
|
|
||
| void syncWorkspaceSessions(structuralSummariesRef.current, activeSessionId).catch((error) => { | ||
| if (!active) { | ||
| return | ||
| } | ||
|
|
||
| console.error('[WorkspaceSessionSync] Failed to sync session resources', error) | ||
| }) | ||
|
|
||
| return () => { | ||
| active = false | ||
| } | ||
| }, [activeSessionId, syncKey, syncWorkspaceSessions]) |
There was a problem hiding this comment.
Apply the snapshots returned by syncWorkspaceSessions.
This effect ignores the WorkspaceSessionSnapshot[] it just fetched and waits for later status pushes instead. SYNC is the IPC path that returns snapshots for every session but is not described as publishing them, so worktree/preview fields can stay stale until some later event happens.
💡 Suggested fix
useEffect(() => {
let active = true
- void syncWorkspaceSessions(structuralSummariesRef.current, activeSessionId).catch((error) => {
- if (!active) {
- return
- }
-
- console.error('[WorkspaceSessionSync] Failed to sync session resources', error)
- })
+ void syncWorkspaceSessions(structuralSummariesRef.current, activeSessionId)
+ .then((snapshots) => {
+ if (!active) {
+ return
+ }
+
+ void Promise.all(
+ snapshots.map((snapshot) => applyWorkspaceSessionSnapshot(snapshot))
+ ).catch((error) => {
+ if (!active) {
+ return
+ }
+
+ console.error('[WorkspaceSessionSync] Failed to apply synced session snapshots', error)
+ })
+ })
+ .catch((error) => {
+ if (!active) {
+ return
+ }
+
+ console.error('[WorkspaceSessionSync] Failed to sync session resources', error)
+ })
return () => {
active = false
}
- }, [activeSessionId, syncKey, syncWorkspaceSessions])
+ }, [activeSessionId, applyWorkspaceSessionSnapshot, syncKey, syncWorkspaceSessions])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessionSync.ts` around
lines 39 - 53, The effect calls
syncWorkspaceSessions(structuralSummariesRef.current, activeSessionId) but
ignores its returned WorkspaceSessionSnapshot[]; update the effect to await the
promise result (or handle the resolved value) and apply those snapshots to the
local session store/state so worktree/preview fields are updated immediately
rather than waiting for later pushes—use the same active check (active flag)
before applying, and invoke the existing session update logic (the function or
setter that updates sessions/state used elsewhere in this hook) so snapshots
from syncWorkspaceSessions are merged/applied for each session (match by session
id) including updating worktree/preview fields.
| const ensureHost = useCallback(async (): Promise<OpenCodeHostStatus | null> => { | ||
| setBusy(true) | ||
| ensureStartRef.current = performance.now() | ||
|
|
||
| try { | ||
| const nextStatus = await window.xnetOpenCode.ensure() | ||
| setStatus(nextStatus) | ||
| return nextStatus | ||
| } catch (error) { | ||
| const normalized = error instanceof Error ? error : new Error(String(error)) | ||
| setLastOutput(normalized.message) | ||
| telemetry.reportCrash(normalized, { | ||
| codeNamespace: 'electron.workspace', | ||
| codeFunction: 'workspace.opencode.ensure' | ||
| }) | ||
| return null | ||
| } finally { | ||
| setBusy(false) | ||
| } | ||
| }, [telemetry]) | ||
|
|
||
| useEffect(() => { | ||
| let active = true | ||
|
|
||
| void window.xnetOpenCode.status().then((nextStatus) => { | ||
| if (!active) { | ||
| return | ||
| } | ||
|
|
||
| setStatus(nextStatus) | ||
| }) |
There was a problem hiding this comment.
Turn startup failures into UI state.
The initial status() call is uncaught, and the ensure() catch only updates lastOutput. If the host fails before a STATUS_CHANGE event is emitted, this panel can stay on the boot screen forever instead of showing the error state. Re-query status() or synthesize an error status in the failure path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/renderer/workspace/OpenCodePanel.tsx` around lines 52 - 82,
The initial call to window.xnetOpenCode.status() in the useEffect is not
handling failures, so if the host fails before a STATUS_CHANGE event the UI
stays stuck; update the effect to catch errors from window.xnetOpenCode.status()
and set an error-like OpenCodeHostStatus (or call setStatus with a synthesized
failure status) and setLastOutput/telemetry as in ensure()’s catch; also in
ensureHost’s catch, after reporting telemetry and setLastOutput, call
window.xnetOpenCode.status() (or setStatus to the same synthesized error status)
so the UI state reflects the failure—refer to ensureHost, useEffect,
window.xnetOpenCode.status, setStatus, setLastOutput and telemetry.reportCrash
to locate where to add the try/catch and the synthesized error status update.
| const [capturingScreenshot, setCapturingScreenshot] = useState(false) | ||
| const [creatingPullRequest, setCreatingPullRequest] = useState(false) | ||
| const [prStatus, setPrStatus] = useState<string | null>(null) |
There was a problem hiding this comment.
Keep screenshot/PR state session-scoped.
prStatus, capturingScreenshot, and creatingPullRequest are shared across whichever session is currently rendered, and both async handlers keep writing them after awaits. If the user starts a screenshot or PR on session A and switches to session B, session A's completion can overwrite B's panel state. Reset these values on activeSession.id changes or gate late completions by the captured session id.
Also applies to: 83-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/renderer/workspace/PreviewWorkspace.tsx` around lines 58 -
60, The state variables capturingScreenshot, creatingPullRequest, and prStatus
are not scoped to a specific session so asynchronous completions from a previous
session can overwrite the UI for a newly selected session; update
PreviewWorkspace to either reset these states whenever activeSession.id changes
(e.g., in a useEffect watching activeSession.id) or capture the current session
id at the start of each async handler and ignore/set state only if the captured
id still matches activeSession.id when the async work completes; apply this
gating pattern to the screenshot handler and PR creation handlers that reference
capturingScreenshot, creatingPullRequest, and setPrStatus so late completions
cannot update the wrong session's panel.
| window.parent.postMessage( | ||
| { | ||
| type: PREVIEW_SELECTED_CONTEXT_MESSAGE_TYPE, | ||
| routeId: readAttribute(routeTarget, 'data-xnet-route-id') ?? routeId, | ||
| targetId: readAttribute(target, 'data-xnet-target-id'), | ||
| targetLabel: readAttribute(target, 'data-xnet-target-label'), | ||
| fileHint: readAttribute(target, 'data-xnet-file-hint'), | ||
| documentId: readAttribute(documentTarget, 'data-xnet-document-id'), | ||
| bounds: { | ||
| x: bounds.x, | ||
| y: bounds.y, | ||
| width: bounds.width, | ||
| height: bounds.height | ||
| }, | ||
| nearbyText: buildNearbyText(target) | ||
| }, | ||
| '*' | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the PreviewContextBridge.tsx file
find . -name "PreviewContextBridge.tsx" -type fRepository: crs48/xNet
Length of output: 104
🏁 Script executed:
# Check the full implementation of the file
cat -n apps/web/src/components/PreviewContextBridge.tsxRepository: crs48/xNet
Length of output: 2725
🏁 Script executed:
# Search for origin or parent validation patterns in the codebase
rg -i "targetOrigin|parent.*origin|allowed.*origin" --type ts --type tsx -B 2 -A 2Repository: crs48/xNet
Length of output: 81
🏁 Script executed:
# Check for environment variables or configuration related to origins
rg -i "parent|origin|host" apps/web/src --type ts --type tsx | grep -i "env\|config\|process" | head -20Repository: crs48/xNet
Length of output: 81
🏁 Script executed:
# Search for any security-related handling of postMessage
rg "postMessage" --type ts --type tsx -B 3 -A 3Repository: crs48/xNet
Length of output: 81
🏁 Script executed:
# Search for postMessage usage and any origin validation
rg "postMessage" -B 3 -A 3Repository: crs48/xNet
Length of output: 50368
🏁 Script executed:
# Look for environment variables or configuration that might define allowed origins
rg "PARENT|ORIGIN|HOST" --type ts | head -20Repository: crs48/xNet
Length of output: 1801
🏁 Script executed:
# Search for any configuration files that might define allowed origins
find apps/web -name "*.env*" -o -name "*.config*" -o -name "tsconfig*" | head -10Repository: crs48/xNet
Length of output: 183
🏁 Script executed:
# Check if there are any comments or documentation about parent origins
rg "parent.*origin|allowed.*origin|targetOrigin" -i -B 2 -A 2Repository: crs48/xNet
Length of output: 5759
🏁 Script executed:
# Look for similar postMessage patterns to see if origin validation is done elsewhere
rg "window\.parent|window\.top" -B 5 -A 5Repository: crs48/xNet
Length of output: 2019
Specify an explicit parent origin instead of using '*' for postMessage.
Line 63 broadcasts the message to any parent origin using '*', even though the payload contains sensitive context: fileHint, documentId, bounds, and nearbyText. In an embedded iframe scenario, this leaks page structure, content snippets, and document metadata to any malicious parent. Thread an allowed parent origin from the host environment and skip posting when unavailable. The codebase already demonstrates this pattern in packages/plugins/src/services/local-api.ts with configurable allowedOrigins validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/PreviewContextBridge.tsx` around lines 47 - 64, The
postMessage call that sends PREVIEW_SELECTED_CONTEXT_MESSAGE_TYPE currently uses
'*' and can leak sensitive data; update the sender to resolve an explicit
allowed parent origin (e.g., from the host environment or a shared config
similar to packages/plugins/src/services/local-api.ts's allowedOrigins) and
replace the '*' targetOrigin with that resolved origin in the
window.parent.postMessage call in PreviewContextBridge; if no allowed parent
origin is available, do not call window.parent.postMessage (skip posting) and
ensure this change is applied where readAttribute(routeTarget,...),
readAttribute(target,...), readAttribute(documentTarget,...) and
buildNearbyText(target) are used to build the payload so the message is only
delivered to a validated origin.
| ### Preload / Service Boundary Mismatch | ||
|
|
||
| There is also a concrete prerequisite bug to fix before leaning on managed services heavily. | ||
|
|
||
| Observed mismatch: | ||
|
|
||
| - `apps/electron/src/main/service-ipc.ts:53-128` registers `start`, `stop`, `restart`, `status`, `list-all`, `call`, and emits `status-update` and `output` | ||
| - `packages/plugins/src/services/client.ts:12-20` expects exactly those channels | ||
| - but `apps/electron/src/preload/index.ts:240-267` allowlists only `start`, `stop`, `status`, and `list` | ||
|
|
||
| Implication: | ||
|
|
||
| - a renderer-driven OpenCode service client will be artificially blocked until the preload allowlist is corrected | ||
|
|
There was a problem hiding this comment.
This prerequisite note is already outdated.
The same PR updates apps/electron/src/preload/index.ts to gate service channels through isAllowedServiceChannel and exposes the missing bridge APIs, but this section and checklist still describe the preload/service mismatch as unresolved. Please mark it as historical context or completed so readers do not chase a bug that is already fixed.
Also applies to: 614-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@docs/explorations/0108_`[_]_ELECTRON_FIRST_SELF_EDITING_WORKSPACE_SHELL_CODEX_LIKE_CHATS_WORKTREES_BRANCHES_PREVIEW_DIFF_PRS_OPENCODE_INTEGRATION.md
around lines 108 - 121, Update the "Preload / Service Boundary Mismatch" section
in the document to mark the mismatch as historical/completed: note that the
preload now gates channels via isAllowedServiceChannel and that the missing
bridge APIs were added (so renderer-driven OpenCode service client is no longer
blocked), and remove or mark completed the checklist items that reference the
old allowlist mismatch (the references to start/stop/status/list vs the
service-ipc channels). Ensure the doc text reflects the fix and/or moves it to
historical context so readers don't treat it as an unresolved bug.
- reserve preview ports across concurrent warm-session startups - confine renderer-supplied worktree paths to managed worktree containers - cover stopped-host, branch parsing, and managed-path regressions with tests
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/electron/src/main/preview-manager.test.ts (1)
70-83: Consider exposingallocatePortfor testing or using a dedicated test helper.The type coercion to access the private
allocatePortmethod works but is fragile. If the internal API changes, this test will fail at runtime without compile-time warnings.Alternative approaches:
- Export
allocatePortas a public method if it's part of the contract- Create a test-specific subclass or factory that exposes internals
This is a minor concern; the current approach is acceptable for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/main/preview-manager.test.ts` around lines 70 - 83, The test accesses a private method via type coercion (allocatePort) on the object returned by createPreviewManager which is fragile; update the code to make testing robust by either exporting allocatePort as a public method on the PreviewManager API or adding a test-only helper/factory that returns an instance exposing allocatePort (e.g., createPreviewManagerTestInstance or a TestPreviewManager subclass) so the test can call manager.allocatePort('session-1') and manager.allocatePort('session-2') without unsafe type casts; adjust the test to use the new exported symbol instead of coercing the type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/electron/src/main/preview-manager.test.ts`:
- Around line 70-83: The test accesses a private method via type coercion
(allocatePort) on the object returned by createPreviewManager which is fragile;
update the code to make testing robust by either exporting allocatePort as a
public method on the PreviewManager API or adding a test-only helper/factory
that returns an instance exposing allocatePort (e.g.,
createPreviewManagerTestInstance or a TestPreviewManager subclass) so the test
can call manager.allocatePort('session-1') and manager.allocatePort('session-2')
without unsafe type casts; adjust the test to use the new exported symbol
instead of coercing the type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e34a6ef2-6986-4f21-bb1c-44a9c9957e28
📒 Files selected for processing (7)
apps/electron/src/__tests__/opencode-host-controller.test.tsapps/electron/src/main/git-service.test.tsapps/electron/src/main/git-service.tsapps/electron/src/main/opencode-host-controller.tsapps/electron/src/main/preview-manager.test.tsapps/electron/src/main/preview-manager.tsapps/electron/src/main/workspace-session-ipc.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/electron/src/main/git-service.test.ts
- apps/electron/src/main/git-service.ts
| window.parent.postMessage( | ||
| { | ||
| type: PREVIEW_SELECTED_CONTEXT_MESSAGE_TYPE, | ||
| routeId: readAttribute(routeTarget, 'data-xnet-route-id') ?? routeId, | ||
| targetId: readAttribute(target, 'data-xnet-target-id'), | ||
| targetLabel: readAttribute(target, 'data-xnet-target-label'), | ||
| fileHint: readAttribute(target, 'data-xnet-file-hint'), | ||
| documentId: readAttribute(documentTarget, 'data-xnet-document-id'), | ||
| bounds: { | ||
| x: bounds.x, | ||
| y: bounds.y, | ||
| width: bounds.width, | ||
| height: bounds.height | ||
| }, | ||
| nearbyText: buildNearbyText(target) | ||
| }, | ||
| '*' | ||
| ) |
There was a problem hiding this comment.
postMessage uses wildcard target origin
window.parent.postMessage(data, '*') delivers the message to any parent frame regardless of its origin. The payload contains user-readable data (nearbyText, targetId, documentId, bounds) that should only be visible to the trusted Electron shell.
While usePreviewSelectedContext.ts validates event.origin on the receiving side, the sending side still broadcasts to all possible parents. If the preview app is opened standalone in a browser (e.g., during development or QA), or if the Electron renderer is ever compromised by a cross-origin iframe injected via XSS, a malicious parent frame can silently receive every right-click context dump.
Use a specific target origin instead of '*'. Since PreviewContextBridge runs inside the preview at a known local origin, you can key off window.location.origin or an environment-injected allowed-parent constant:
window.parent.postMessage(
{ ... },
window.location.origin // restrict delivery to same-origin parent
)If the shell and the preview run on different local origins (different ports), pass the parent origin as a prop or inject it at build time.
| export function parseGitFileChanges(output: string): GitFileChange[] { | ||
| return output | ||
| .split('\n') | ||
| .map((line) => line.trimEnd()) | ||
| .filter(Boolean) | ||
| .map((line) => ({ | ||
| status: line.slice(0, 2).trim() || '??', | ||
| path: line.slice(3).trim() | ||
| })) |
There was a problem hiding this comment.
Renamed file paths parsed as old -> new string
In git status --porcelain=v1 output, renamed files are represented as:
R old-path -> new-path
line.slice(3).trim() produces old-path -> new-path as the path field. This raw string is then:
- Displayed verbatim in
ChangedFilesPanel— the user sees a->arrow instead of an actual filename. - Passed to
readRelativeFilefor markdown preview —join(cwd, 'old-path -> new-path')is a non-existent path; the silent.catch(() => null)masks the failure.
For renames, only the destination path is meaningful. Split on -> and take the last segment:
export function parseGitFileChanges(output: string): GitFileChange[] {
return output
.split('\n')
.map((line) => line.trimEnd())
.filter(Boolean)
.map((line) => {
const rawPath = line.slice(3).trim()
// Porcelain v1 represents renames as "old -> new"; extract destination
const arrowIdx = rawPath.indexOf(' -> ')
const path = arrowIdx !== -1 ? rawPath.slice(arrowIdx + 4) : rawPath
return {
status: line.slice(0, 2).trim() || '??',
path
}
})
}| export class GitService { | ||
| private repoContextPromise: Promise<GitRepoContext> | null = null | ||
|
|
||
| constructor(private readonly options: GitServiceOptions = {}) {} | ||
|
|
||
| async resolveRepoContext(startPath?: string): Promise<GitRepoContext> { | ||
| if (!startPath && this.repoContextPromise) { | ||
| return this.repoContextPromise | ||
| } | ||
|
|
||
| const task = this.resolveRepoContextInternal(startPath) | ||
| if (!startPath) { | ||
| this.repoContextPromise = task | ||
| } | ||
|
|
||
| return task | ||
| } |
There was a problem hiding this comment.
Cached repoContextPromise never cleared on rejection
repoContextPromise is assigned the in-flight task before it settles. If resolveRepoContextInternal rejects (e.g. git rev-parse fails because the binary is missing or the CWD is not a repo), repoContextPromise holds a permanently rejected Promise. Every subsequent call to resolveRepoContext() will return the same rejected promise without retrying, making all workspace operations fail until the app is restarted.
Add a rejection handler that clears the cached promise so the next call retries:
const task = this.resolveRepoContextInternal(startPath)
if (!startPath) {
this.repoContextPromise = task
task.catch(() => {
this.repoContextPromise = null
})
}
return task- render simple menu labels as static text instead of Base UI group labels - prevent SystemMenu from crashing the Electron shell on open - keep advanced dropdown group label exports unchanged for grouped menus
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/primitives/Menu.tsx (1)
117-118: Add an Electron regression test for the simple label path.This fixes a shell-crash path, so it’s worth covering with a Playwright case that opens a menu using
MenuLabeland asserts the menu renders without crashing.Based on learnings: Test UI changes in Electron with Playwright after implementing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/primitives/Menu.tsx` around lines 117 - 118, Add an Electron Playwright regression test that reproduces the simple label path using the MenuLabel component: write a test (e.g., "menu-label-electron.spec") that launches the Electron app, opens a menu that contains MenuLabel, interacts to trigger rendering of that menu (simulate click/hover as your UI requires), and asserts the menu renders without crashing by checking for the presence of the label text or a menu DOM node; ensure the test runs in the Electron runner (not browser only), waits for stability, and fails if the renderer or main process crashes so this shell-crash path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/primitives/Menu.tsx`:
- Around line 117-118: Add an Electron Playwright regression test that
reproduces the simple label path using the MenuLabel component: write a test
(e.g., "menu-label-electron.spec") that launches the Electron app, opens a menu
that contains MenuLabel, interacts to trigger rendering of that menu (simulate
click/hover as your UI requires), and asserts the menu renders without crashing
by checking for the presence of the label text or a menu DOM node; ensure the
test runs in the Electron runner (not browser only), waits for stability, and
fails if the renderer or main process crashes so this shell-crash path is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e0e2676-1539-47b8-a996-8e08e037f6b7
📒 Files selected for processing (1)
packages/ui/src/primitives/Menu.tsx
| async function findAvailableManagedPort( | ||
| host: string, | ||
| startPort: number, | ||
| claimedPorts: ReadonlySet<number> | ||
| ): Promise<number> { | ||
| let port = startPort | ||
|
|
||
| while (claimedPorts.has(port) || !(await isPortAvailable(host, port))) { | ||
| port += 1 | ||
| } | ||
|
|
||
| return port | ||
| } |
There was a problem hiding this comment.
findAvailableManagedPort has no upper-bound guard
The while loop increments port indefinitely until it finds an available port. If all ports in the range from basePort through 65535 are occupied (or claimed), this function will attempt to probe ports beyond the valid range (>65535), likely throwing an OS error or spinning into an infinite loop.
A simple safety net is to cap the search at the maximum valid port:
| async function findAvailableManagedPort( | |
| host: string, | |
| startPort: number, | |
| claimedPorts: ReadonlySet<number> | |
| ): Promise<number> { | |
| let port = startPort | |
| while (claimedPorts.has(port) || !(await isPortAvailable(host, port))) { | |
| port += 1 | |
| } | |
| return port | |
| } | |
| async function findAvailableManagedPort( | |
| host: string, | |
| startPort: number, | |
| claimedPorts: ReadonlySet<number> | |
| ): Promise<number> { | |
| let port = startPort | |
| const MAX_PORT = 65535 | |
| while (port <= MAX_PORT && (claimedPorts.has(port) || !(await isPortAvailable(host, port)))) { | |
| port += 1 | |
| } | |
| if (port > MAX_PORT) { | |
| throw new Error(`No available port found starting from ${startPort}`) | |
| } | |
| return port | |
| } |
| async getDiffStat(cwd: string): Promise<string> { | ||
| const result = await this.runGit(['diff', '--stat', '--no-ext-diff'], cwd) | ||
| return trimCommandOutput(result.stdout) | ||
| } | ||
|
|
||
| async getDiffPatch(cwd: string): Promise<string> { | ||
| const result = await this.runGit(['diff', '--no-ext-diff'], cwd) | ||
| return result.stdout | ||
| } |
There was a problem hiding this comment.
getDiffStat/getDiffPatch show only unstaged changes, making PR review empty after commits
git diff --stat and git diff with no extra arguments only show the diff between the working tree and the index (i.e., unstaged changes). Once a user commits their work, both functions return empty strings, so the PR review panel displays "No diff summary available" and an empty diff patch — even though there may be many committed changes on the branch.
For a meaningful PR review surface, these should compare against the worktree session's base ref rather than showing only the transient working-tree state:
async getDiffStat(cwd: string, baseRef?: string): Promise<string> {
const args = baseRef
? ['diff', '--stat', '--no-ext-diff', `${baseRef}..HEAD`]
: ['diff', '--stat', '--no-ext-diff']
const result = await this.runGit(args, cwd)
return trimCommandOutput(result.stdout)
}
async getDiffPatch(cwd: string, baseRef?: string): Promise<string> {
const args = baseRef
? ['diff', '--no-ext-diff', `${baseRef}..HEAD`]
: ['diff', '--no-ext-diff']
const result = await this.runGit(args, cwd)
return result.stdout
}The buildWorkspaceReview caller in workspace-session-ipc.ts has the session.branch and baseRef available from the repo context and could pass them through.
| const bulletFiles = changedFiles.slice(0, 10).map((file) => `- ${file.path}`) | ||
|
|
There was a problem hiding this comment.
PR draft file list silently truncated at 10 entries
changedFiles.slice(0, 10) caps the bullet list at 10 files but produces no indication that more files exist. If a session has 20 changed files, the PR body will list only the first 10, and the reviewer will have no idea there are more. Consider appending a summary line when the list is truncated:
| const bulletFiles = changedFiles.slice(0, 10).map((file) => `- ${file.path}`) | |
| const bulletFiles = changedFiles.slice(0, 10).map((file) => `- ${file.path}`) | |
| const truncatedCount = changedFiles.length - 10 | |
| if (truncatedCount > 0) { | |
| bulletFiles.push(`- _…and ${truncatedCount} more file${truncatedCount === 1 ? '' : 's'}_`) | |
| } |
apps/electron/src/renderer/workspace/hooks/useWorkspaceReview.ts
Outdated
Show resolved
Hide resolved
- create missing workspace session summaries when runtime status arrives before local state exists - route session create and sync flows through snapshot upsert logic to avoid renderer races - add a regression test for the missing-summary status event path
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx (1)
23-25:⚠️ Potential issue | 🟡 MinorKeep the identity stable across the remount test.
Line 24 generates a new identity on every
renderWorkspaceHooks()call, so the second mount at Line 152 changes two variables at once: storage is reused, but the XNet identity is not. That weakens this as a persistence/remount assertion and can hide identity-scoped restore bugs.🧪 Suggested fixture fix
-function createWrapper(storage: MemoryNodeStorageAdapter) { - const identity = generateIdentity() +function createWrapper( + storage: MemoryNodeStorageAdapter, + identity = generateIdentity() +) { const consent = new ConsentManager() consent.setTier('anonymous') const collector = new TelemetryCollector({ consent }) @@ -function renderWorkspaceHooks(storage = new MemoryNodeStorageAdapter()) { +function renderWorkspaceHooks( + storage = new MemoryNodeStorageAdapter(), + identity = generateIdentity() +) { return renderHook( @@ - wrapper: createWrapper(storage) + wrapper: createWrapper(storage, identity) } ) } @@ it('restores session summaries and active selection from local xNet state after remount', async () => { const storage = new MemoryNodeStorageAdapter() - const initialRender = renderWorkspaceHooks(storage) + const identity = generateIdentity() + const initialRender = renderWorkspaceHooks(storage, identity) @@ - const remounted = renderWorkspaceHooks(storage) + const remounted = renderWorkspaceHooks(storage, identity)Also applies to: 127-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx` around lines 23 - 25, createWrapper currently calls generateIdentity() each time, so renderWorkspaceHooks remount tests reuse storage but get a different XNet identity; make the identity stable across remounts by creating a single identity fixture and reusing it for subsequent renderWorkspaceHooks calls (e.g., lift generateIdentity() out of createWrapper or accept an identity param), ensuring createWrapper/ConsentManager still initializes per-render but the XNet identity remains constant to properly assert persistence/restoration behavior.
🧹 Nitpick comments (2)
apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx (2)
11-11: Drop the defaultReactimport here.Line 11 is the only default import in the file, and the repo rule for TS/TSX is named exports only. If the renderer is already on the automatic JSX runtime,
import { type ReactNode, useMemo } from 'react'is enough here.♻️ Suggested import cleanup
-import React, { type ReactNode, useMemo } from 'react' +import { type ReactNode, useMemo } from 'react'As per coding guidelines:
**/*.{ts,tsx,js,jsx}: Use named exports only; no default exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx` at line 11, Remove the default React import and switch to named imports: change the import statement that currently uses default React to only import the named symbols used (e.g., "type ReactNode" and "useMemo"); locate the import line referencing React and replace it with a named-only import so it reads like "import { type ReactNode, useMemo } from 'react'" to comply with the repo rule enforcing named exports for TS/TSX.
77-192: Add one Playwright path for this restore flow in the real Electron shell.These hook tests are useful, but they will not catch preload/IPC/router wiring regressions in the actual workspace UI. A single Electron Playwright smoke test for create → remount/restore would cover the highest-risk integration path from this PR.
Based on learnings: Test UI changes in Electron with Playwright after implementing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx` around lines 77 - 192, Add a high-level Playwright smoke test that exercises the real Electron shell for the "create → remount/restore" flow (similar to the hook test "restores session summaries and active selection from local xNet state after remount"): launch the packaged/dev Electron app, use the UI or preload IPC to invoke the equivalent of ensureWorkspaceShellState and createSessionSummary (or simulate the UI flow that creates a session), quit/unmount the app, relaunch it, and assert the session list and active selection are restored (check session title and active ID). Ensure the test runs against the same storage backend used by MemoryNodeStorageAdapter in tests (or point Electron to that persisted store) and fails on IPC/preload/router regressions by verifying the DOM and IPC-visible state after remount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx`:
- Around line 23-25: createWrapper currently calls generateIdentity() each time,
so renderWorkspaceHooks remount tests reuse storage but get a different XNet
identity; make the identity stable across remounts by creating a single identity
fixture and reusing it for subsequent renderWorkspaceHooks calls (e.g., lift
generateIdentity() out of createWrapper or accept an identity param), ensuring
createWrapper/ConsentManager still initializes per-render but the XNet identity
remains constant to properly assert persistence/restoration behavior.
---
Nitpick comments:
In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx`:
- Line 11: Remove the default React import and switch to named imports: change
the import statement that currently uses default React to only import the named
symbols used (e.g., "type ReactNode" and "useMemo"); locate the import line
referencing React and replace it with a named-only import so it reads like
"import { type ReactNode, useMemo } from 'react'" to comply with the repo rule
enforcing named exports for TS/TSX.
- Around line 77-192: Add a high-level Playwright smoke test that exercises the
real Electron shell for the "create → remount/restore" flow (similar to the hook
test "restores session summaries and active selection from local xNet state
after remount"): launch the packaged/dev Electron app, use the UI or preload IPC
to invoke the equivalent of ensureWorkspaceShellState and createSessionSummary
(or simulate the UI flow that creates a session), quit/unmount the app, relaunch
it, and assert the session list and active selection are restored (check session
title and active ID). Ensure the test runs against the same storage backend used
by MemoryNodeStorageAdapter in tests (or point Electron to that persisted store)
and fails on IPC/preload/router regressions by verifying the DOM and IPC-visible
state after remount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31aa7338-5940-4848-a9ae-0ff6b3bcea27
📒 Files selected for processing (2)
apps/electron/src/renderer/workspace/hooks/useSessionCommands.tsapps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/electron/src/renderer/workspace/hooks/useSessionCommands.ts
| const ensured = await controller.ensure() | ||
| if (ensured.state !== 'ready' && ensured.state !== 'starting') { | ||
| throw new Error('OpenCode host is not available for prompt prefill') | ||
| } | ||
|
|
||
| const config = createOpenCodeHostConfig(process.env) | ||
| await waitForOpenCodeHealth(config) | ||
|
|
There was a problem hiding this comment.
Prompt append may fail on cold starts due to short health poll window
When controller.ensure() returns with state === 'starting', the code immediately calls waitForOpenCodeHealth, which polls at most 20 times with a 250 ms sleep between probes. Each probeOpenCodeHealth call has a 2000 ms abort timeout, so when opencode is still starting and connections are refused (fast rejection), the effective max wait is roughly 20 × 250 ms = 5 seconds. On slower machines or a cold start with dependency resolution, opencode may legitimately need more than 5 seconds before its HTTP server is ready, causing appendPrompt to throw and the prefill to be silently lost.
Consider increasing the attempt count or extracting the timeout as a configurable constant:
const HEALTH_PROBE_ATTEMPTS = 40 // 40 × 250 ms = 10 s
const HEALTH_PROBE_INTERVAL_MS = 250
async function waitForOpenCodeHealth(config: OpenCodeHostConfig): Promise<void> {
for (let attempt = 0; attempt < HEALTH_PROBE_ATTEMPTS; attempt += 1) {
...
}
throw new Error('OpenCode host is not ready to accept prompt updates')
}Alternatively, the caller in APPEND_PROMPT could distinguish between a timeout failure and other errors and retry transparently.
| async resolveRepoContext(startPath?: string): Promise<GitRepoContext> { | ||
| if (!startPath && this.repoContextPromise) { | ||
| return this.repoContextPromise | ||
| } | ||
|
|
||
| const task = this.resolveRepoContextInternal(startPath) | ||
| if (!startPath) { | ||
| this.repoContextPromise = task | ||
| } | ||
|
|
||
| return task | ||
| } |
There was a problem hiding this comment.
Cached repoContextPromise is never cleared on rejection
repoContextPromise is assigned the in-flight task before it settles (line 270). If resolveRepoContextInternal rejects (e.g., git rev-parse fails because the binary is missing or the process CWD is not inside a repo), repoContextPromise holds a permanently rejected Promise. Every subsequent call to resolveRepoContext() without a startPath returns the same rejected promise immediately, making all workspace operations fail silently until the app restarts.
Add a rejection handler that clears the cache so the next call retries:
const task = this.resolveRepoContextInternal(startPath)
if (!startPath) {
this.repoContextPromise = task
task.catch(() => {
this.repoContextPromise = null
})
}
return task- suppress identical workspace session status publishes in Electron main - no-op identical renderer snapshot applies to avoid repeated xNet writes - stop workspace review refreshes from keying off incidental session object churn - add regression coverage for idempotent snapshot application
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/electron/src/renderer/workspace/hooks/useWorkspaceReview.ts`:
- Around line 48-85: The refresh callback in useWorkspaceReview captures
activeSession and is listed as a dependency, causing refresh to be recreated
when activeSession reference changes; replace that by storing activeSession in a
ref (e.g., activeSessionRef) and update the ref whenever activeSession changes,
then remove activeSession from refresh's dependency array and read
activeSessionRef.current inside refresh (use it when calling
reviewWorkspaceSession and when providing sessionId to telemetry.reportCrash);
keep reviewWorkspaceSession and telemetry in the dependency array so callback
identity is stable while still using the latest session via the ref.
In `@apps/electron/src/renderer/workspace/state/active-session.ts`:
- Line 56: The value assigned to changedFilesCount (input.changedFilesCount) can
become NaN/Infinity after Math.trunc, which bypasses the integer/min schema
checks; update the construction in active-session.ts to coerce and validate the
raw input first (e.g., read Number(input.changedFilesCount ?? 0) or use
Number.isFinite on the numeric coercion), then apply Math.trunc and Math.max(0,
…) to that finite number; do the same defensive check for the other occurrence
around lines 106-107 so changedFilesCount is always a finite non-negative
integer before being stored.
- Around line 165-176: matchesWorkspaceSessionSnapshot currently compares raw
session fields to snapshot strings, causing whitespace-only differences to
appear as changes; update the comparison to first normalize both sides the same
way used by the create/patch helpers (trim strings and collapse optional empties
to the canonical empty value used there) for fields such as title, branch,
worktreeName, worktreePath, openCodeUrl, previewUrl, lastScreenshotPath,
lastError and state; ensure you apply the same normalization helper or inline
logic before comparing (referencing matchesWorkspaceSessionSnapshot, session,
and snapshot) so redundant updates due to whitespace/optional-empty mismatches
are avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 244dffea-9f5d-4039-9624-162ad25b2fda
📒 Files selected for processing (6)
apps/electron/src/main/workspace-session-ipc.tsapps/electron/src/renderer/workspace/hooks/useSessionCommands.tsapps/electron/src/renderer/workspace/hooks/useWorkspaceReview.tsapps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsxapps/electron/src/renderer/workspace/state/active-session.tsapps/electron/src/shared/workspace-session.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/electron/src/main/workspace-session-ipc.ts
- apps/electron/src/renderer/workspace/hooks/useSessionCommands.ts
- apps/electron/src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx
- apps/electron/src/shared/workspace-session.ts
| lastMessagePreview: input.lastMessagePreview?.trim() || undefined, | ||
| lastScreenshotPath: input.lastScreenshotPath?.trim() || undefined, | ||
| lastError: input.lastError?.trim() || undefined, | ||
| changedFilesCount: Math.max(0, Math.trunc(input.changedFilesCount ?? 0)), |
There was a problem hiding this comment.
Guard changedFilesCount against NaN/Infinity.
Line 56 and Line 107 clamp negatives, but they still let malformed numeric values through: Math.max(0, Math.trunc(NaN)) is NaN. That can break the schema’s integer/min contract and keep retriggering writes for a bad snapshot.
Proposed fix
+function normalizeChangedFilesCount(value: number | undefined): number {
+ if (!Number.isFinite(value)) {
+ return 0
+ }
+
+ return Math.max(0, Math.trunc(value))
+}
+
export function createSessionSummaryInput(
input: SessionSummaryInput
): InferCreateProps<(typeof SessionSummarySchema)['_properties']> {
return {
@@
- changedFilesCount: Math.max(0, Math.trunc(input.changedFilesCount ?? 0)),
+ changedFilesCount: normalizeChangedFilesCount(input.changedFilesCount),
@@
export function createSessionSummaryPatch(
patch: Partial<SessionSummaryInput>
): Partial<InferCreateProps<(typeof SessionSummarySchema)['_properties']>> {
@@
if (patch.changedFilesCount !== undefined) {
- next.changedFilesCount = Math.max(0, Math.trunc(patch.changedFilesCount))
+ next.changedFilesCount = normalizeChangedFilesCount(patch.changedFilesCount)
}Also applies to: 106-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/renderer/workspace/state/active-session.ts` at line 56, The
value assigned to changedFilesCount (input.changedFilesCount) can become
NaN/Infinity after Math.trunc, which bypasses the integer/min schema checks;
update the construction in active-session.ts to coerce and validate the raw
input first (e.g., read Number(input.changedFilesCount ?? 0) or use
Number.isFinite on the numeric coercion), then apply Math.trunc and Math.max(0,
…) to that finite number; do the same defensive check for the other occurrence
around lines 106-107 so changedFilesCount is always a finite non-negative
integer before being stored.
| return ( | ||
| (session.title ?? '') === snapshot.title && | ||
| (session.branch ?? '') === snapshot.branch && | ||
| (session.worktreeName ?? '') === snapshot.worktreeName && | ||
| (session.worktreePath ?? '') === snapshot.worktreePath && | ||
| (session.openCodeUrl ?? '') === snapshot.openCodeUrl && | ||
| (session.previewUrl ?? null) === (snapshot.previewUrl ?? null) && | ||
| (session.lastScreenshotPath ?? null) === (snapshot.lastScreenshotPath ?? null) && | ||
| (session.lastError ?? null) === (snapshot.lastError ?? null) && | ||
| (session.changedFilesCount ?? 0) === snapshot.changedFilesCount && | ||
| Boolean(session.isDirty) === snapshot.isDirty && | ||
| (session.state ?? 'idle') === snapshot.state |
There was a problem hiding this comment.
Compare against normalized snapshot values.
This equality check uses raw snapshot strings, while the create/patch helpers trim and collapse optional empties. A snapshot that only differs by whitespace will fail matchesWorkspaceSessionSnapshot() and still trigger a redundant update, which undercuts the churn-deduping this PR is aiming for.
Proposed fix
export function matchesWorkspaceSessionSnapshot(
session: SessionSummaryNode,
snapshot: WorkspaceSessionSnapshot
): boolean {
+ const normalized = createSessionSummaryInput(
+ createSessionSummaryInputFromWorkspaceSnapshot(snapshot)
+ )
+
return (
- (session.title ?? '') === snapshot.title &&
- (session.branch ?? '') === snapshot.branch &&
- (session.worktreeName ?? '') === snapshot.worktreeName &&
- (session.worktreePath ?? '') === snapshot.worktreePath &&
- (session.openCodeUrl ?? '') === snapshot.openCodeUrl &&
- (session.previewUrl ?? null) === (snapshot.previewUrl ?? null) &&
- (session.lastScreenshotPath ?? null) === (snapshot.lastScreenshotPath ?? null) &&
- (session.lastError ?? null) === (snapshot.lastError ?? null) &&
- (session.changedFilesCount ?? 0) === snapshot.changedFilesCount &&
- Boolean(session.isDirty) === snapshot.isDirty &&
- (session.state ?? 'idle') === snapshot.state
+ (session.title ?? '') === normalized.title &&
+ (session.branch ?? '') === normalized.branch &&
+ (session.worktreeName ?? '') === normalized.worktreeName &&
+ (session.worktreePath ?? '') === normalized.worktreePath &&
+ (session.openCodeUrl ?? '') === normalized.openCodeUrl &&
+ (session.previewUrl ?? undefined) === normalized.previewUrl &&
+ (session.lastScreenshotPath ?? undefined) === normalized.lastScreenshotPath &&
+ (session.lastError ?? undefined) === normalized.lastError &&
+ (session.changedFilesCount ?? 0) === normalized.changedFilesCount &&
+ Boolean(session.isDirty) === normalized.isDirty &&
+ (session.state ?? 'idle') === normalized.state
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electron/src/renderer/workspace/state/active-session.ts` around lines
165 - 176, matchesWorkspaceSessionSnapshot currently compares raw session fields
to snapshot strings, causing whitespace-only differences to appear as changes;
update the comparison to first normalize both sides the same way used by the
create/patch helpers (trim strings and collapse optional empties to the
canonical empty value used there) for fields such as title, branch,
worktreeName, worktreePath, openCodeUrl, previewUrl, lastScreenshotPath,
lastError and state; ensure you apply the same normalization helper or inline
logic before comparing (referencing matchesWorkspaceSessionSnapshot, session,
and snapshot) so redundant updates due to whitespace/optional-empty mismatches
are avoided.
| <iframe | ||
| title="Preview" | ||
| src={activeSession.previewUrl} | ||
| className="h-full w-full border-0 bg-white" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Preview iframe missing sandbox attribute
Without a sandbox attribute, the embedded Vite preview can navigate the parent (Electron renderer) window to an arbitrary URL, open new top-level windows, and submit forms. In Electron this means a compromised or misbehaving dev server at the allocated localhost port could navigate the renderer window away from the shell, or load file:// content in a popup.
Adding sandbox="allow-scripts allow-forms allow-same-origin allow-popups" (adjusted to your use-case) constrains what the iframe can do. At minimum, allow-top-navigation should be omitted to prevent the preview from hijacking the parent frame.
| <iframe | |
| title="Preview" | |
| src={activeSession.previewUrl} | |
| className="h-full w-full border-0 bg-white" | |
| /> | |
| </div> | |
| <div className="h-full overflow-hidden rounded-[28px] border border-border/70 bg-background shadow-2xl shadow-black/10"> | |
| <iframe | |
| title="Preview" | |
| src={activeSession.previewUrl} | |
| sandbox="allow-scripts allow-forms allow-same-origin allow-popups allow-modals" | |
| className="h-full w-full border-0 bg-white" | |
| /> | |
| </div> |
| async readRelativeFile(cwd: string, relativePath: string): Promise<string> { | ||
| const absolutePath = join(cwd, relativePath) | ||
| return readFile(absolutePath, 'utf8') | ||
| } |
There was a problem hiding this comment.
readRelativeFile does not bound-check path within the worktree
join(cwd, relativePath) can silently escape cwd if relativePath contains ../ segments. While standard git status --porcelain=v1 output uses repository-relative paths (no ..), a symlink in the worktree pointing outside it, or an unusual git repository layout, could produce a path that resolves outside the expected worktree directory. The resolved path is then passed directly to readFile.
A simple containment guard prevents this:
async readRelativeFile(cwd: string, relativePath: string): Promise<string> {
const absolutePath = resolve(join(cwd, relativePath))
const normalizedCwd = resolve(cwd)
if (!absolutePath.startsWith(normalizedCwd + sep) && absolutePath !== normalizedCwd) {
throw new Error(`readRelativeFile: path escapes worktree root: ${absolutePath}`)
}
return readFile(absolutePath, 'utf8')
}(sep and resolve are already imported in the file.)
apps/electron/src/renderer/workspace/hooks/useWorkspaceSessionSync.ts
Outdated
Show resolved
Hide resolved
- keep workspace snapshot apply callbacks stable across session summary updates - read latest session summaries from refs to avoid sync effect self-trigger loops - stabilize review refresh callbacks so panel fetches key off structural changes
| runtime.readinessTask = withTimeout(this.waitForReady(runtime), this.readyTimeoutMs) | ||
| await runtime.readinessTask | ||
|
|
||
| if (runtime.state !== 'error') { | ||
| runtime.state = 'ready' | ||
| runtime.startedAt = Date.now() | ||
| runtime.lastError = null | ||
| this.startHealthChecks(runtime) | ||
| this.emitStatus(session.sessionId) | ||
| } | ||
| } catch (error) { | ||
| if (runtime.process) { | ||
| runtime.process.kill('SIGKILL') | ||
| } | ||
| runtime.state = 'error' | ||
| runtime.lastError = formatCommandFailure( | ||
| 'pnpm', | ||
| ['exec', 'vite', '--host', this.host, '--port', String(port), '--strictPort'], | ||
| previewAppPath, | ||
| error | ||
| ) | ||
| this.emitStatus(session.sessionId) | ||
| } | ||
|
|
||
| return this.getStatus(session.sessionId) |
There was a problem hiding this comment.
stopRequested not checked after readinessTask resolves — orphaned health timer
When stopSession is called while a session is still in the starting state, runtime.stopRequested is set to true, causing waitForReady to exit its polling loop early. The readinessTask promise then resolves, but startSession continues executing and reaches the block below without checking stopRequested:
if (runtime.state !== 'error') {
runtime.state = 'ready'
runtime.startedAt = Date.now()
runtime.lastError = null
this.startHealthChecks(runtime) // ← timer leak: fires forever on a deleted runtime
this.emitStatus(session.sessionId)
}Because the exit handler deletes the session from this.runtimes, the health interval created here keeps calling probePreviewUrl indefinitely without any code ever cleaning it up. The only way to stop it would be an app restart.
The fix is to gate the transition on !runtime.stopRequested:
if (!runtime.stopRequested && runtime.state !== 'error') {
runtime.state = 'ready'
runtime.startedAt = Date.now()
runtime.lastError = null
this.startHealthChecks(runtime)
this.emitStatus(session.sessionId)
}| async createCommit(cwd: string, message: string): Promise<void> { | ||
| await this.runGit(['commit', '-m', message], cwd) | ||
| } |
There was a problem hiding this comment.
createCommit does not stage working-tree changes before committing
git commit -m message only commits files that are already in the index. If the caller has not explicitly run git add, this command will either fail with "nothing to commit" or silently commit only previously-staged files, leaving all working-tree modifications uncommitted.
For the intended PR workflow — where OpenCode edits files and then a PR is drafted from those changes — calling createCommit without first staging everything would produce an empty or partial commit, leading to a PR with no meaningful diff.
If the intent is to stage all tracked modifications automatically, the implementation should run git add -A (or git add -u if untracked files should be excluded) before the commit:
async createCommit(cwd: string, message: string): Promise<void> {
await this.runGit(['add', '-A'], cwd)
await this.runGit(['commit', '-m', message], cwd)
}If the caller is always expected to handle staging independently, this constraint should be documented with a comment.
- decouple workspace sync and review effects from callback identity churn - stabilize telemetry hook return values for renderer command usage - add a regression test for equivalent rerenders in workspace session sync
- add gated renderer logs for workspace sync requests and status events - log snapshot noop, update, create, and failure decisions during sync - reuse localStorage debug flags so normal app usage stays quiet
- keep workspace sync ordering stable so keep-warm previews do not churn - dedupe repeated runtime snapshot writes while identical updates are in flight - add regression coverage for snapshot dedupe and summary reorder resyncs - alias workspace package exports to source in web Vite dev for preview worktrees
- pass the source checkout root into worktree preview Vite processes - resolve workspace package aliases from that source root during preview startup - avoid broken @fs module loads from isolated worktree package sources - validate the coding workspace flow in Electron with clean-profile automation
|
|
||
| try { | ||
| const nextServiceStatus = current | ||
| ? await deps.restartService(OPENCODE_SERVICE_ID) | ||
| : await deps.startService( | ||
| createOpenCodeServiceDefinition({ ...config, binaryPath: resolution.path }) | ||
| ) |
There was a problem hiding this comment.
restartService invoked for cleanly-stopped services
When the service is in the 'stopped' state (i.e., it was previously started and then cleanly shut down via stop()), current is truthy and the ternary at line 148 calls deps.restartService instead of deps.startService. If the underlying process manager's restartService implementation tries to send a kill signal to the previous process before starting a new one, it will attempt to kill a process that no longer exists, which can throw or silently fail — leaving the service in a partially-initialised state.
The guard in ensure() now correctly exits early unless current.state === 'error' OR current.state === 'stopped'. However, the restart/start selection below doesn't distinguish between those two cases:
// Only call restartService when the process is still alive (error state).
// For a cleanly stopped service, startService is the correct call.
const nextServiceStatus =
current && current.state === 'error'
? await deps.restartService(OPENCODE_SERVICE_ID)
: await deps.startService(
createOpenCodeServiceDefinition({ ...config, binaryPath: resolution.path })
)| private async startSession(session: WorkspaceSessionDescriptor): Promise<PreviewRuntimeStatus> { | ||
| const port = await this.allocatePort(session.sessionId) | ||
| const previewAppPath = join(session.worktreePath, 'apps', 'web') | ||
| const previewAppExists = await fileExists(previewAppPath) | ||
| if (!previewAppExists) { | ||
| const runtime = this.createRuntime(session, port) | ||
| this.releaseReservedPort(session.sessionId) | ||
| runtime.state = 'error' | ||
| runtime.lastError = `Preview app not found at ${previewAppPath}` | ||
| this.runtimes.set(session.sessionId, runtime) | ||
| this.emitStatus(session.sessionId) | ||
| return this.getStatus(session.sessionId) | ||
| } | ||
|
|
||
| const runtime = this.createRuntime(session, port) | ||
| this.releaseReservedPort(session.sessionId) |
There was a problem hiding this comment.
Orphaned Vite process when stopAll races with startSession
startSession has two await points — allocatePort() and fileExists() — that occur before this.runtimes.set(session.sessionId, runtime) is called. If stopAll() (e.g., triggered by app shutdown) fires during either of those awaits, it iterates over this.runtimes which does not yet contain the in-progress session. After stopAll() resolves, the suspended startSession continuation resumes, inserts the runtime into the (now-empty) Map, and spawns the Vite child process — with no code path that will ever kill it.
On app quit this means a stray pnpm exec vite process is left running in the background.
A minimal fix is to check runtime.stopRequested (or a separate module-level "shutting down" flag) immediately after each await in startSession and abort before spawning:
const port = await this.allocatePort(session.sessionId)
if (this.shuttingDown) return this.getStatus(session.sessionId)
const previewAppExists = await fileExists(previewAppPath)
if (this.shuttingDown) return this.getStatus(session.sessionId)| const ensured = await controller.ensure() | ||
| if (ensured.state !== 'ready' && ensured.state !== 'starting') { | ||
| throw new Error('OpenCode host is not available for prompt prefill') | ||
| } | ||
|
|
||
| const config = createOpenCodeHostConfig(process.env) | ||
| await waitForOpenCodeHealth(config) |
There was a problem hiding this comment.
Redundant health poll when service is already 'ready'
waitForOpenCodeHealth is called unconditionally even when ensured.state === 'ready', meaning every call to appendPrompt performs at least one HTTP round-trip to the health endpoint before the actual prompt POST. When OpenCode is already healthy, this adds unnecessary latency to each prompt prefill.
Consider skipping the poll when the service has already been confirmed healthy:
if (ensured.state === 'starting') {
await waitForOpenCodeHealth(config)
}
// If ensured.state === 'ready', the controller already probed health — skip the wait.| <div className="h-full overflow-hidden rounded-[28px] border border-border/70 bg-background shadow-2xl shadow-black/10"> | ||
| <iframe | ||
| title="Preview" | ||
| src={activeSession.previewUrl} | ||
| className="h-full w-full border-0 bg-white" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
captureWorkspaceScreenshot captures the full Electron window
browserWindow.webContents.capturePage() (called in the main-process handler) captures the entire renderer window, including the session rail, the OpenCode chat panel, and any other visible UI — not just the Vite preview iframe shown here. This means every screenshot stored under tmp/playwright/${sessionId}.png will contain the complete workspace shell, potentially exposing chat history, file names, and other session metadata in the artifact committed alongside the PR body.
If the intent is to capture only the preview content, capturePage accepts a rect argument and you could use the iframe's bounding rect. Alternatively, document this behavior so users are aware the screenshot includes the full shell.
1 participant
Summary
Testing
cd apps/electron && pnpm vitest run src/main/command-errors.test.ts src/main/git-service.test.ts src/main/preview-manager.test.ts src/renderer/workspace/performance.test.ts src/renderer/workspace/state/active-session.test.ts src/renderer/workspace/hooks/useWorkspaceSessions.test.tsx src/renderer/workspace/SessionRail.test.tsxcd apps/electron && pnpm buildpnpm vitest run --changed HEAD --passWithNoTestspnpm turbo run typecheck --affectedgit pushpre-push hook:pnpm typecheck && pnpm testbetter-sqlite3for Electron arm64Notes
pnpm run deps:electron/@electron/rebuildutil.styleTextworkaround inapps/electron/README.mdpnpm run dev:hubstill hits an existing local Node/esbuild architecture mismatch in this environmentSummary by CodeRabbit
New Features
Documentation
Tests
Security