feat(desktop): persist and restore UI state across restarts#578
feat(desktop): persist and restore UI state across restarts#578pascalandr wants to merge 29 commits into
Conversation
Restore the primary Electron or Tauri client's window, zoom, workspace tabs, active sessions, drafts, bounded attachments, scroll positions, and panel layout across launches. Secondary processes continue to start normally without reading or writing the shared snapshot. Add versioned atomic native persistence, process ownership election, renderer capability tokens, bounded flush handshakes for shutdown and navigation, and translated settings to disable or clear restored state. Preserve future snapshot envelopes and merge partial or timed-out workspace hydration without resurrecting explicit user deletions. Correlate restore-created workspaces and harden startup cancellation and shutdown so late events cannot create ghost tabs or orphan child processes. Verify process identities before platform-specific cleanup, bound retries and shutdown, and report incomplete cleanup with a nonzero exit. Cover UI reconciliation and persistence races, Electron election and lifecycle behavior, Tauri navigation and shutdown, and server workspace process cleanup. Validated typechecks, native and focused regression suites, Electron build, and Tauri release build.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29092029490 Artifacts expire in 7 days.
|
Resolve bare Windows commands such as the npm-installed opencode shim through PATH and PATHEXT, then launch cmd, PowerShell, and native executables through the matching spawn path. Manage direct executables through their owned ChildProcess and stop script wrappers with bounded taskkill tree cleanup instead of repeated all-process CIM queries. Preserve incomplete cleanup state when taskkill fails so retries and shutdown cannot silently accept orphaned descendants or target a reused PID. Add regression coverage for the observed opencode.cmd timeout, direct executables, taskkill escalation and failure, wrapper exit races, retry behavior, and shutdown reporting.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29098938563 Artifacts expire in 7 days.
|
Keep virtualized message anchors under bounded stabilization until their measured offset settles, and prevent content growth from overwriting a valid restored position. Cancel restoration on explicit user navigation and guard callbacks across session changes. Restore the persisted Tauri zoom-in shortcut using the actual Ctrl/Cmd+Plus accelerator chord instead of Ctrl/Cmd+Equal. Add regression coverage for delayed and unstable anchor mounts, content churn, bounded fallback, cancellation, stale generations, and top-crossing anchor selection.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29103593207 Artifacts expire in 7 days.
|
Re-enable WebView zoom hotkeys and restore the original Windows View-menu labels for Ctrl++ and Ctrl+-. Remove the replacement menu accelerators that did not match the user's keyboard behavior.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29105202649 Artifacts expire in 7 days.
|
Observe WebView2 zoom-factor changes so native keyboard and wheel controls update the cached window zoom without replacing the existing shortcuts or menu labels. Primary windows capture and debounce the updated native factor into client state, while secondary windows keep only their local zoom cache current. Invalid native values are rejected and valid values use the existing clamp policy. Add normalization coverage and pin the Windows WebView2 callback dependency to the version used by the current Tauri runtime.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29106447067 Artifacts expire in 7 days.
|
Persist per-client unseen idle timestamps alongside restored workspace state so completed sessions retain their green indicators across restarts. Treat loaded runtime sessions as authoritative to prevent acknowledged markers from reappearing, while preserving markers for sessions unavailable during partial hydration. Validate and bound marker records in the client-state codec and reactively capture session changes. Cover codec filtering, acknowledged-marker tombstones, and partial-restore preservation with targeted tests. UI typecheck, production build, and focused state restoration suites pass.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29151919434 Artifacts expire in 7 days.
|
Persist active-generation recovery state across desktop restarts and reconcile it only against authoritative OpenCode status. Running sessions reconnect passively, unknown status remains pending, and confirmed idle sessions surface a localized interruption notice without replaying prompts or tools. Provide an explicit continuation action that preserves existing and recalled prompt text and only prepares a review-first draft. Track prompt, slash-command, and shell admission with race-safe grouped tokens so SSE updates, overlapping requests, status-fetch races, pending input, and concurrent deletion remain authoritative. Add bounded snapshot codec and partial-restore merge coverage, recovery/admission race tests, translations for every supported locale, and responsive square-corner styling. Validated with UI typecheck, 75 focused tests, production build, diff checks, and a final targeted review with no findings.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper Review
Findings
High: Tauri rejects the outgoing renderer's final state flush during CLI restart
References: packages/tauri-app/src-tauri/src/client_state/commands.rs:43-50, packages/tauri-app/src-tauri/src/cli_manager.rs:1063-1103, packages/tauri-app/src-tauri/src/client_state/navigation.rs:228-260
Impact: Recent drafts, attachments, tab selection, scroll position, and generation-recovery state can be lost during a Tauri CLI restart, directly violating this PR's state-restoration guarantee.
Scenario:
- The main WebView is displaying the old CLI origin, for example
http://127.0.0.1:4000. - Restarting the CLI clears the managed URL and later stores its new origin, for example
http://127.0.0.1:5000. mark_readyupdatesstatus.urlto the new origin before callingnavigate_main.- The navigation worker asks the still-loaded old renderer to flush.
- Every flush command calls
validate_main_window, which compares the old WebView URL against only the newstatus.url;client_state_saveis rejected. - The renderer cannot acknowledge the flush, navigation waits one second, then proceeds. Any state dirtied while the CLI was restarting exists only in the destroyed renderer.
This is especially reproducible by changing a draft or tab while the CLI is restarting, when ordinary debounced writes are also rejected because manager.status().url is temporarily absent or already points at the replacement server.
Required fix: Decouple client-state authorization from the CLI manager's next target URL. Track the origin currently committed in the main WebView separately and retain it through the pre-navigation flush. Rotate that origin only after navigation commits. During a transition, allowing the explicitly tracked outgoing and target origins is reasonable because the main-window label and renderer capability token still gate access. Add an integration-level test covering old origin -> status URL changes -> outgoing renderer save/ack -> navigation -> token rotation.
Better Implementation Opportunities
-
Separate restoration orchestration from capture/persistence.
packages/ui/src/lib/hooks/use-app-session-restore.tsis approximately 601 lines and currently owns hydration, timeout handling, preservation merging, event subscriptions, native flush hooks, and capture scheduling. Splitting restore execution from ongoing capture would make shutdown and partial-hydration races substantially easier to audit. -
Reduce process-lifecycle monoliths.
packages/server/src/workspaces/runtime.tsis approximately 1,029 lines and mixes launch parsing, platform identity discovery, token cleanup, signal escalation, liveness probing, and Windows-specific termination. The implementation is carefully guarded, but its size makes safety invariants difficult to verify. Platform-specific stop controllers would provide clearer ownership boundaries. -
Avoid tying security policy to mutable service status. The Tauri bug above results from using
CliProcessManager::status()simultaneously as UI status and renderer authorization state. A dedicated navigation-origin authority would be simpler and less race-prone. -
Add a supported UI test command. The focused UI tests require browser export conditions, but
packages/ui/package.jsonhas no test script encoding that environment. A repository-supported command would prevent locally valid tests from failing under Node's default server export resolution.
Validation Performed
- Reviewed the complete
ca06bd99...8dcf07dddiff and relevant surrounding Electron, Tauri, server, and UI code. git diff --check ca06bd99...HEAD: passed.- UI, Electron, and server TypeScript typechecks: passed.
- Electron native suite: 43/43 passed.
- Server changed suites: 72/72 passed.
- Tauri Rust suite: 56/56 passed.
- UI production build: passed, with existing large-chunk warnings.
- Electron production build: passed.
- Focused UI invocation: 121 tests passed, one skipped. Browser-conditioned tests pass, but the current direct harness retains an open handle, reinforcing the need for a supported test command.
- Latest GitHub checks observed at posting time: Electron/Linux, Windows, and macOS builds passed; several Tauri jobs remain in progress.
Gatekeeper Verdict
REQUEST_CHANGES
The outgoing Tauri renderer is unauthorized precisely when its pre-navigation flush is needed during CLI-origin rotation. That creates a confirmed state-loss path in the core behavior introduced by this PR and must be fixed before merge.
Posted as a comment review because GitHub does not allow a PR author to formally request changes on their own PR.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29152779456 Artifacts expire in 7 days.
|
Authorize client-state commands against the renderer origin committed with its capability token instead of the CLI manager's mutable target URL. This keeps the outgoing document authorized while a replacement CLI origin is becoming ready. Model navigation as a committed and pending origin transition. The outgoing token remains valid through flush and accepted-but-uncommitted navigation, while the incoming claim atomically rotates both token and origin. Unsupported opaque origins are rejected and Tauri/asset origins remain distinct. Cover changed-manager URL authorization, outgoing flush persistence, incoming token rotation, failed navigation preservation, same-origin reload behavior, and opaque-origin separation. Cargo check and all 57 Tauri tests pass; targeted security review reports no findings.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper Re-Review
Findings
High: Native Windows workspace executables can leave descendant processes orphaned
References:
packages/server/src/workspaces/runtime.ts:885-920
packages/server/src/workspaces/runtime.ts:932-965
packages/server/src/workspaces/spawn.ts:259-267
packages/server/src/workspaces/runtime.test.ts:178-192
Executables ending in .exe or .com are classified as windows-direct. Their shutdown path calls only child.kill("SIGTERM" | "SIGKILL") and considers cleanup successful as soon as that direct child exits. Unlike the wrapper path, it neither invokes taskkill /T nor proves that descendants exited.
On Windows, ChildProcess.kill() terminates the selected process, not its process tree. If opencode.exe has spawned shells, tools, language servers, or other descendants, closing a workspace or shutting down CodeNomad can therefore report successful cleanup while those descendants remain alive. This regresses the base implementation, which used taskkill /PID <pid> /T for Windows launches.
The new test at runtime.test.ts:178-192 explicitly asserts that direct executables never use tree cleanup, so it codifies the unsafe behavior rather than covering descendant cleanup.
Required fix: Put every Windows workspace launch, including direct executables, in a per-workspace Job Object configured with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. As a smaller interim fix, use /T for direct executables too and do not resolve shutdown until tree cleanup is confirmed. Add a test where a native executable exits while a descendant remains and require the descendant to be cleaned up before stop() resolves.
Prior Blocker
The previous Tauri outgoing-renderer flush blocker is resolved at 921d49ae.
The client-state token is now authorized against its committed renderer origin rather than the CLI manager's mutable URL. Navigation records a pending origin, preserves the outgoing token through its flush, and rotates authority only when the incoming origin claims access. Failed navigation and opaque-origin cases are covered. No remaining bypass or flush-loss path was found in that transition.
Better Implementation Opportunities
packages/ui/src/lib/hooks/use-app-session-restore.ts:91-601combines capture, reconciliation, workspace creation, SideCar restoration, native flush registration, timeout handling, and lifecycle cleanup. Extracting native flush registration and restore orchestration into separate modules would make race invariants substantially easier to audit and test.- The Windows process model should use one ownership primitive for direct executables and wrappers. The current
windows-directversuswindows-wrappersplit creates two different cleanup guarantees for processes serving the same workspace role.
Validation Performed
- Confirmed the worktree is at pushed HEAD
921d49ae8123ef9ebf6247e95ec54db2691c1093. - Reviewed the complete
ca06bd99..921d49aediff across 150 files. git diff --check ca06bd99..921d49ae: passed.- UI, Electron, and server TypeScript typechecks: passed.
- Electron native tests: 43 passed.
- Tauri
cargo test: 57 passed. - Targeted changed server tests: 72 passed.
- Broader server tests: 158 passed, with one unrelated failure in unchanged
git-worktrees.test.ts. - Broader UI tests: 203 passed, one skipped, with three test-runner failures caused by client-only Solid APIs loading under the server-side Node test environment.
Gatekeeper Verdict
REQUEST_CHANGES
The prior Tauri blocker is fixed, but the Windows direct-executable shutdown regression can leave workspace descendants running after cleanup is reported successful. That lifecycle and resource-leak risk remains release-blocking.
Posted as a comment review because GitHub does not allow a PR author to formally request changes on their own PR.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29153922647 Artifacts expire in 7 days.
|
Capture immutable CIM CreationDate identities for direct Windows executables before publishing a workspace. Stop direct process trees through guarded identity-matched CIM termination instead of child-only signals or unguarded numeric-PID taskkill calls. Emit every selected identity before termination begins and retain observed targets even when the guarded command fails. This keeps reparented descendants tracked across partial failures, escalation, and liveness confirmation without risking termination of a reused PID. Reject direct launches when ownership cannot be established, while preserving the existing bounded taskkill path for shell wrappers. Server typecheck and 64 focused workspace/process identity tests pass; targeted gatekeeper review reports no findings.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Final Gatekeeper Review
Actionable Findings
None. No blocking regression or correctness issue was found in the complete ca06bd99..f122209b diff.
Prior Blockers
- Resolved: Tauri outgoing renderer flush across CLI origin rotation. Authorization is now tied to the renderer's committed origin rather than the CLI manager's mutable URL. The outgoing document remains authorized during flush, while the incoming claim commits the pending origin and rotates the token. Failed navigation preserves the old authority. References:
packages/tauri-app/src-tauri/src/client_state/access.rs:16-108,commands.rs:48-70,navigation.rs:237-288. - Resolved: direct Windows executable cleanup. Direct launches capture CIM
CreationDateidentity before publication, discover descendants while the verified leader exists, and retain exact identities for later escalation or reparenting. References:packages/server/src/workspaces/runtime.ts:279-297,590-607,674-752. - Resolved: Windows PID-reuse safety. Signals are issued only after matching PID and creation identity. The Node child process handle prevents the launched PID from being reused while launch identity is captured. References:
packages/server/src/workspaces/process-identity.ts:305-337,runtime.ts:775-883. - Resolved: guarded partial failures. All selected Windows targets are emitted before termination starts, and parsed observations survive nonzero command results so descendants remain tracked after partial termination. References:
packages/server/src/workspaces/process-identity.ts:230-269,329-336,runtime.ts:674-688. - Resolved: failed launch publication. Direct Windows launches without immutable identity are rejected before workspace publication, with bounded and retryable cleanup behavior. References:
packages/server/src/workspaces/runtime.ts:279-297,packages/server/src/workspaces/manager.ts:273-321.
Broader Audit
- Persistence ownership, atomic replacement, future-envelope suppression, disable/clear rollback, and shutdown draining are consistently guarded in Electron and Tauri.
- Restore startup gating, cancellation, late workspace creation cleanup, duplicate-folder reconciliation, authoritative deletion, draft/attachment preservation, scroll restoration generations, and interrupted-generation recovery have explicit race handling.
- Web clients remain secondary and do not write native snapshots.
- Electron, Tauri, Windows direct/wrapper, WSL, Linux, and portable POSIX paths remain distinct where their ownership guarantees differ.
- No i18n or styling-policy regressions were found.
Better Implementation Opportunities
These are non-blocking:
- A native Windows Job Object would eventually be preferable to CIM/PowerShell discovery. It would provide kernel-enforced descendant ownership and avoid rejecting direct launches when CIM is unavailable.
- Tauri renderer authority could eventually use a navigation/document generation in addition to origin and token. The current implementation is correct for normal operation, but generation binding would make same-origin document transitions more explicit.
- Several touched source files are beyond the repository's size guidance and should be decomposition candidates:
packages/ui/src/stores/instances.tsat approximately 1,687 lines;packages/server/src/workspaces/runtime.tsat approximately 1,015 lines;packages/ui/src/lib/hooks/use-app-session-restore.tsat approximately 601 lines;packages/electron-app/electron/main/client-state-process.tsat approximately 562 lines.
Residual Risks
- Windows process-tree behavior is comprehensively mocked but was not exercised against real CIM termination, process reparenting, or PID churn on a Windows host.
- Native shutdown and renderer navigation races are unit-tested state machines rather than full packaged Electron/Tauri end-to-end tests.
- An ad hoc aggregate UI Node test invocation could not load two browser-dependent test files because
solid-toastreached Solid's server build. This is a runner/environment limitation, not an observed product failure; the UI typecheck and the other 121 selected tests passed.
Validation
git diff --check ca06bd99 f122209b: passed.- UI, server, and Electron TypeScript typechecks: passed.
- Electron native suite: 43 passed.
- Tauri
cargo test: 57 passed. - Focused server workspace, process-identity, shutdown, spawn, route, and settings suites: 74 passed.
- Selected UI persistence, restore, reconciliation, codec, attachment, scroll, and recovery tests: 121 passed, 1 skipped; two browser-dependent files were not runnable under the ad hoc Node server environment.
Gatekeeper Verdict
APPROVE
Posted as a comment review because GitHub does not allow a PR author to formally approve their own PR.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29154627915 Artifacts expire in 7 days.
|
Retain the latest per-instance stream status on the server and replay it to newly connected event clients. This closes the startup window where an OpenCode bridge connected before the restored renderer subscribed. Preserve an already-received connected status when attaching the SDK client, clear matching disconnect UI on recovery, and serialize a session refresh for every connection notification. The refresh runs after initial hydration so interrupted-generation authority is reconciled even after transient status-fetch failures or reconnects during an active refresh. Add coverage for late status replay, terminal-status eviction, pre-attachment connection seeding, in-flight reconnects, failed refresh retries, and settlement-boundary notifications. UI/server typechecks, 70 focused tests, and the production UI build pass; targeted review reports no findings.
Merge upstream dev at 3470622 into the primary app-state restoration branch so PR NeuralNomadsAI#578 remains mergeable and can produce fresh installable artifacts. Combine dev's arbitrary-depth session tree, ancestor expansion, and list-selection APIs with restored drafts, active-session authority, interrupted-generation recovery, and admission-race handling. Fork selection now retains draft persistence while using the new tree navigation model. Validated with UI and server typechecks, 66 focused UI tests, 52 focused server tests, the production UI build, whitespace checks, and a targeted conflict-resolution review with no findings.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29174756541 Artifacts expire in 7 days.
|
Keep a successfully launched workspace visible when restoring its session data times out or rejects. Register its runtime tab before hydration, preserve unresolved drafts and recovery state, and release restore cleanup ownership instead of deleting the ready workspace. Continue discarding workspaces for non-timeout cancellation so shutdown and explicit abort cleanup remain safe. Add regression coverage for retaining a ready workspace on hydration timeout. Validated with the UI typecheck, 57 focused restoration and recovery tests, the production UI build, whitespace checks, and a targeted review with no findings.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29184109750 Artifacts expire in 7 days.
|
Move snapshot serialization, preservation merging, debounced persistence, and native flush hooks into useAppSessionCapture. Keep useAppSessionRestore focused on reconstructing workspace and SideCar tabs while retaining the same public hook in App. Preserve the original startup ordering by preparing capture, releasing the restore gate, and only then activating reactive persistence. This keeps partial-restore state and shutdown flush behavior unchanged. Validated with the UI typecheck, 58 focused restoration and client-state tests, the production UI build, whitespace checks, and a targeted lifecycle review.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29185498456 Artifacts expire in 7 days.
|
Persist recursive session expansion per workspace and hydrate it without dropping IDs for sessions that load later. Runtime expansion authority prevents collapsed sessions from being reopened, while explicit deletion removes stale expansion entries. Display unresolved restored work as provisionally active in session and workspace indicators without changing raw runtime status, replaying generation work, or acquiring a wake lock. Once OpenCode reports idle, keep the terminal interrupted marker. Replace the interrupted-generation continuation action with a square amber informational tag and localized copy. No prompt is prefilled and no resume behavior remains. Validated with the UI typecheck, 72 focused codec/restoration/status/tree tests, the production UI build, whitespace checks, and a targeted review with no findings.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29187969488 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29200989553 Artifacts expire in 7 days.
|
Integrate canonical realpath workspace reuse and the latest dev build changes without weakening restore cancellation, process cleanup, or duplicate-tab semantics. Restore existing tabs provisionally, hydrate independent workspaces and SideCars concurrently, defer noncritical workspace metadata, and avoid eager message loading for hidden sessions. Preserve user selection, ordering, drafts, attachments, and session choices while background restoration completes. Correlate restore-created workspace ownership with request IDs so cancellation cannot delete reused workspaces, and cover canonical alias sharing, readiness cleanup, release validation, and disposal races. Validated with UI and server typechecks, 125 focused restore/workspace/runtime tests, repeated identity-concurrency runs, whitespace checks, and a production Vite build.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29248981048 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29249707241 Artifacts expire in 7 days.
|
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Consolidated gatekeeper review
Reviewed against head 5bb35663. CI is green, but the following regressions remain in platform and shutdown paths that the current tests do not exercise end to end.
P1
-
Windows identity capture rejects a normal snapshot containing PID 0.
probeWindowsProcesses()serializes everyWin32_Processrow (packages/server/src/workspaces/process-identity.ts:400-411), whileparseDelimitedSnapshot()rejects the entire output when any PID is<= 0(:167-188). Windows exposes the System Idle Process as PID 0, so direct Windows launches can fail atruntime.ts:279-292even when the spawned PID is present. Filter PID 0 before parsing or skip that record without invalidating the snapshot. -
The portable POSIX snapshot can exceed its one-second launch timeout. On macOS/BSD,
POSIX_SNAPSHOT_SCRIPTruns threepscommands plus base64 work for every process (process-identity.ts:78-91), but launch identity capture gives the whole scan onlystopCommandTimeoutMs, defaulting to 1000 ms (runtime.ts:171,279-292). A moderately populated process table can therefore reject an otherwise healthy workspace launch. Capture all needed columns in a bounded number of commands or use a launch-specific probe. -
Spawn failures can emit an unhandled child
error. The permanent listener is not attached untilruntime.ts:392, after synchronous identity capture. If spawn returns a child without a PID (for exampleENOENT/EACCES), the function takes the early return at:284-292;beginFailedLaunchCleanup()does not install an error listener. The deferred child-processerrorevent can then crash the server. Attach the listener immediately after spawn and route the early identity failure through the same finalizer. -
Graceful shutdown leaves remote proxy listeners alive. Each
RemoteProxySessionManagersession starts its own Fastify HTTPS listener (packages/server/src/server/remote-proxy.ts:41-49,132-154), butorchestrateServerShutdown()has no operation for this manager andindex.ts:571-589never disposes it. Because successful shutdown now sets an exit code rather than forcing process termination, active proxy sessions can keep the process alive. Add a manager-wide shutdown method and await it during orchestration. -
A queued loading-screen navigation can destroy a newer CLI preload.
showLoadingScreen()queues a navigation whose callback later destroys the currentpreloadingViewand clearspendingCliUrl(packages/electron-app/electron/main/main.ts:447-469). If CLI readiness callsstartCliPreload()before that queued callback executes (:481-531), the stale callback tears down the newly created view and no retry remains. Serialize these state transitions by generation or make the callback verify it is still current before mutating preload state. -
A flush can return before the newest queued snapshot write finishes.
flushClientState()captures and awaits the currentwriteQueueonce (packages/ui/src/stores/client-state.ts:293-312). While it is awaiting that promise, a debounced save can append a replacement promise and cleardirtyinenqueuePendingSave()(:171-193). The flush then observesdirty === falseand returns without awaiting the replacement queue, so native shutdown/navigation can be acknowledged before the latest snapshot is durable. Loop until both the queue identity is stable and no dirty state remains.
P2
-
Progressively restored tabs are interactive while capture remains disabled. Capture is enabled only in the outer restore
finally(packages/ui/src/lib/hooks/use-app-session-restore.ts:374-401), afterPromise.all()waits for every workspace and sidecar (:345-354). A restored tab can already be visible and editable while another restoration stalls; page hide during that interval flushes the old snapshot becausecaptureEnabled()is still false (use-app-session-capture.ts:165-196). Enable merged capture as soon as the first restored state is exposed, preserving unresolved entries through the existing preservation layer. -
Restore cancellation does not cancel the workspace launch request. The abort signal is checked only after
serverApi.createWorkspace()resolves (packages/ui/src/stores/abortable-restore-creation.ts:3-17;instances.ts:964-992) and is never passed to the HTTP request or runtime. If the process starts but never reports a port, timeout/cancellation cannot reach it and the request-scoped creation can retain its process and canonical-path reservation indefinitely. Propagate an abort/dispose operation to the server-side pending creation. -
Persisted restore-disabled state does not restore write suppression after restart. Electron loads
state.restoreEnabledbut leavespersistenceSuppressed = false(packages/electron-app/electron/main/client-state.ts:105-134); Tauri does the same withAtomicBool::new(false)(packages/tauri-app/src-tauri/src/client_state.rs:107-133). Native window events can consequently write window state while restoration is disabled, and that stale state becomes eligible when restoration is enabled later. Initialize suppression from the persisted preference in both hosts.
Gatekeeper status: blocked pending the P1 fixes. The P2 items should also be resolved or explicitly accepted with targeted regression coverage.
Make process identity capture reliable on Windows and portable POSIX hosts, and keep spawn failures handled before launch setup completes. Close remote proxy listeners during server shutdown, serialize Electron loading transitions safely, and restore native write suppression from persisted preferences. Keep progressive restores captured, wait for replacement snapshot writes, and propagate restore cancellation through an idempotent request-scoped server cleanup path. Validated with targeted server, UI, Electron, and Tauri tests; server/UI/Electron typechecks; UI and Electron builds; Rust formatting; and git diff checks.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper re-review
Reviewed the corrective commit f975d99c against the previous head and rechecked each finding from review 4692299157.
Findings
No remaining blocking findings in the corrective diff.
Resolved
- Windows process enumeration now filters PID 0 before snapshot parsing.
- macOS/BSD identity capture now uses one bounded process-table query instead of per-PID subprocesses.
- Spawned children receive an error listener immediately, including early identity-failure returns.
- Remote proxy listeners and dispatchers are awaited by the server shutdown orchestration.
- Electron performs destructive loading-state changes before queuing navigation, so stale callbacks cannot destroy a newer preload.
- Client-state flush waits until the write queue identity is stable and the state is clean.
- Capture starts as soon as progressive restore begins, with unresolved state retained by the preservation layer.
- Restore cancellation now aborts the HTTP request and invokes idempotent request-scoped server cleanup, including pre-reservation cancellation.
- Electron and Tauri initialize write suppression from persisted
restoreEnabled: falsestate.
Validation
- 60 targeted server tests passed.
- 44 Electron native tests passed.
- 14 targeted UI tests passed.
- 31 Tauri client-state tests passed.
- Server, UI, and Electron typechecks passed.
- UI and Electron builds passed.
- Touched Rust files pass
rustfmt --check. git diff --checkpassed.
The new GitHub Actions matrix is still running at review time. Gatekeeper result: clear subject to CI completion and the repository-required independent approval.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
General gatekeeper review: changes requested
Reviewed the full diff from 483fac2e through f975d99c, not only the latest corrective commit. GitHub does not allow requesting changes on your own PR, so this is submitted as COMMENTED; the gatekeeper status is blocked.
-
[P1] Prevent remote proxy sessions from being published after shutdown starts (
packages/server/src/server/remote-proxy.ts:132-169).shutdown()snapshots the current map, but an already-runningcreateSession()can finishapp.listen()afterward and insert a new listener at line 154. Since graceful shutdown now only setsprocess.exitCode, that listener keeps the process alive indefinitely after SIGTERM. Mark the manager closed before draining and reject or immediately close creations that cross that boundary, or stop request admission before draining sessions. -
[P1] Do not delete a restore-created workspace after it has become user-visible (
packages/ui/src/stores/instances.ts:984-994,packages/ui/src/lib/hooks/use-app-session-restore.ts:208-236). The create commit immediately callsupsertWorkspace(), while ownership is retained until initial session hydration completes. During the 60-second restore window the user can select that workspace and type or otherwise interact; if hydration later times out, the catch path still callsdisposeRestoreCreatedInstance()and deletes the workspace and its unsaved state. Transfer ownership on the first authoritative user interaction, or keep the workspace hidden/provisional until restore commits. -
[P2] Avoid an unbounded blocking lock during Tauri startup (
packages/tauri-app/src-tauri/src/client_state/process.rs:39).lock_exclusive()runs synchronously inside.setup(). If another process is suspended or wedged while holding the registration lock, every subsequent launch blocks indefinitely before the window is usable. Usetry_lock_exclusive()with bounded retry/timeout and a safe non-primary fallback. -
[P2] Do not let one failed initial hydration permanently poison reconnect resyncs (
packages/ui/src/stores/instances.ts:233-238). The rejected promise remains ininitialHydrations, so every laterinstance.eventStatus: connectedrequest awaits and fails on the same settled rejection without ever reachingfetchSessions(). Neutralize the initial failure before the reconnect path, clear/replace the entry, or only await hydration while it is still pending. -
[P2] Preserve all SSE-updated session fields when merging a stale fetch (
packages/ui/src/stores/session-generation-recovery.ts:41-50). This PR moves the capture ofexistingSessionsahead of the HTTP requests. Ifsession.updatedarrives while those requests are in flight,latest !== captured, but the merge starts from stalefetchedand copies only runtime fields. Newertitle,metadata,time,revert, model, or agent values are therefore rolled back until another update. Merge the complete latest session and overlay only fields that are authoritative from the fetch, or version all mutable fields. -
[P2] Fall back to the parent when an active child is deleted remotely (
packages/ui/src/stores/session-api.ts:748-753). A newsession.deletedSSE event can remove the active child whileactiveParentSessionIdstill points to its valid parent. CallingclearActiveSession()leaves no active conversation instead of selecting that parent, so the content pane goes blank after another client deletes the child. Set the active session to the retained parent when it still exists; clear both only when the parent itself is deleted.
The previously reported nine gatekeeper findings remain fixed in f975d99c. These six additional findings are from the full-PR pass and are not covered by the current targeted tests.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29325936395 Artifacts expire in 7 days.
|
Prevent graceful shutdown from racing with remote proxy creation, bound Tauri registration locking, and transfer restore-created workspaces to user ownership as soon as they are selected. Make reconnect resync recover from failed initial hydration, preserve complete newer SSE session state over stale fetches, and fall back to a retained parent when an active child is deleted remotely. Add regression coverage for each lifecycle and state transition. Validated with targeted server and UI tests, the full Tauri test suite, server/UI typechecks, the UI build, rustfmt, and diff checks.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Full-review remediation re-check
Rechecked commit 9b3cb30e against the six findings from review 4693509023.
Resolved
- Remote proxy shutdown now closes admission, waits for in-flight creations, rejects creations crossing the shutdown boundary, and drains every published listener.
- Selecting a restore-created workspace now immediately transfers it to user ownership, so a later hydration timeout cannot delete a workspace the user has started using.
- Tauri registration locking now uses bounded retry and safely initializes as a secondary process after timeout instead of blocking startup indefinitely.
- Reconnect resync treats failed initial hydration as a recoverable prerequisite and still executes a fresh session fetch.
- Stale fetch merging now preserves the complete latest SSE session object, including title, metadata, timestamps, model, agent, revert, and runtime fields.
- Remote deletion of the active child now selects the retained parent instead of leaving the conversation pane without an active session.
Validation
- 6 remote proxy server tests passed.
- 31 targeted UI regression tests passed.
- 59 Tauri tests passed.
- Server and UI typechecks passed.
- UI production build passed.
- Touched Rust file passes
rustfmt --check. git diff --checkpassed.
No remaining findings in the corrective commit. The refreshed GitHub Actions matrix is running; repository-required independent approval is still pending.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29328221166 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29328894469 Artifacts expire in 7 days. |
Remove the interrupted-generation notice, continuation button, and prompt-prefill API from the message view so restart recovery never presents an action inside the conversation. Restore the persistent localized Interrupted badge for affected session and sub-session rows, with warning styling that replaces the transient Idle presentation. Validated with the UI typecheck, production build, 43 focused recovery and snapshot tests, and diff checks.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Interrupted-generation presentation corrected in ffd9fbed
The conversation view no longer renders an interruption notice, continuation button, or prompt-prefill action. Interrupted recovery is now represented only by a persistent localized amber Interrupted status badge on the affected session or sub-session row, replacing the raw Idle label.
Validation: UI typecheck, production build, 43 focused recovery/snapshot tests, and git diff --check passed.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29334993077 Artifacts expire in 7 days.
|
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper review: changes requested
Reviewed the latest head ffd9fbed against current base 07e8ce68, including the previous lifecycle fixes and the interrupted-generation presentation correction. GitHub does not allow requesting changes on your own PR, so this is submitted as COMMENTED; the gatekeeper status is blocked.
- [P2] Bound the outer Electron lock-recovery loop (
packages/electron-app/electron/main/client-state-process.ts:421-463).acquireProcessOwnerLockWithStatus()correctly returns afterliveOwnerWaitMs, but when a malformed/incomplete lock cannot be removed becauseunlinkSync()keeps returning one of the errors classified as transient (EPERM,EACCES, orEBUSY), it returns{ acquired: false, liveOwner: undefined }. Lines 438-440 immediately restart the helper with a fresh deadline, soClientStateManagerconstruction can block the Electron main thread forever and the app never opens. Use one deadline/attempt budget for the entire marker-backed acquisition and fall back to secondary ownership when it expires. Add a regression test with an injected persistent unlink failure; the current election tests cover live valid owners but not malformed undeletable locks.
Rechecked
- The six findings from review
4693509023remain resolved. - The conversation no longer renders an interrupted-generation notice or continuation action.
- Interrupted sessions now retain a localized warning badge instead of the raw Idle label.
- 37 targeted Electron native tests passed.
- 43 targeted UI recovery/snapshot tests passed.
- UI typecheck passed.
- Windows and Linux CI builds are green; the remaining refreshed platform jobs are still running.
No additional actionable regressions were confirmed. Gatekeeper result: blocked on the unbounded Electron startup loop and pending CI completion/independent approval.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29338278628 Artifacts expire in 7 days.
|
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper verdict: BLOCK.
The head is unchanged at ffd9fbedd6247bf6bd405d0ac75c9e8e16af667d, the worktree is clean, and all CI jobs are green. I used 12 of the authorized 50 subagent sessions (8 primary, 4 nested), then independently revalidated the reported interleavings against the current code. The following issues remain:
-
[P1] Track and bound every remote-proxy disposal (
packages/server/src/server/remote-proxy.ts:187-212).disposeSession()removes a session fromsessionsbefore awaitingAgent.close()andFastify.close(). An idle cleanup can therefore start a disposal, delete the map entry, and then be missed byshutdown()'s snapshot. In addition, neither close has a deadline or abort path; a hijacked SSE/streaming request atproxyRequest()can keep either promise pending indefinitely. A single stale proxy can consequently prevent SIGTERM shutdown from reaching workspaces or the HTTP servers. Keep in-flight disposal promises in a tracked set and make shutdown wait for them with a bounded force-close/abort policy. -
[P1] Do not overwrite an authoritative fetched runtime status with a completed admission (
packages/ui/src/stores/session-generation-recovery.ts:32-44). IffetchSessions()captures an idle session, a prompt admission starts and completes whilesession.status()is in flight,withSession()produces a newer clone whose token is now cleared but whose local status is still idle/pending. Becauselatest !== captured, the unconditional...latestwins over the fetched authoritativeworkingstatus. The UI can report the session as non-working and admit another prompt while generation is already running. Preserve only fields that actually changed authoritatively after capture, and let a known HTTPworking/compactingstatus clear the admission state. -
[P1] Serialize Tauri fallback election and ownership release (
packages/tauri-app/src-tauri/src/client_state/process.rs:42-96,146-161). Two interleavings can leave all live processes secondary. First, while process A holds the registration lock, process B times out, publishes an ordinary live marker, and becomes secondary; when A resumes, it acquires the primary lock but sees B's marker and releases it. Unlike Electron, B's marker does not acknowledge the registering owner, so neither process is primary. Second, shutdown removes A's marker before releasing its primary lock without taking the registration lock; B can register in that gap, fail the primary lock, and remain secondary after A releases it. In both cases persistence silently stops until every process exits. The timeout marker needs explicit acknowledgement semantics, and marker/primary ownership release must participate in the same election serialization. -
[P2] Preserve the registration-lock deadline across Electron retries (
packages/electron-app/electron/main/client-state-process.ts:421-463).acquireProcessOwnerLockWithStatus()has a deadline, but this wrapper immediately starts it again whenever the returned acquisition has no parsedliveOwner. A malformed lock file that repeatedly fails unchanged removal withEPERM,EACCES, orEBUSYtherefore resets the deadline forever whileAtomics.wait()blocks the Electron main thread. Apply one outer deadline/attempt budget and conservatively fall back to secondary when it expires. -
[P2] Flush state on Windows session termination (
packages/electron-app/electron/main/client-state-lifecycle.ts:68-103). The lifecycle only handlesbefore-quit. Electron does not emitbefore-quitwhen Windows ends the session for logout, restart, or shutdown, so the renderer/native flush and primary drain are skipped and the latest snapshot is lost. Register the BrowserWindowquery-session-end/session-endlifecycle and perform a bounded final flush for this platform path. -
[P2] Treat client-state initialization as optional in Tauri (
packages/tauri-app/src-tauri/src/main.rs:605-612). Every app-data, marker, or lock error returned byClientState::initialize()is propagated fromsetup, aborting the entire desktop application. Client-state restore is recoverable state and Electron already degrades conservatively on election failures; a stale permission/lock problem must not prevent CodeNomad from launching. Log the error and manage a non-primary/no-restore state instead. -
[P2] Represent every restore owner of a shared workspace creation (
packages/server/src/workspaces/manager.ts:257-285,498-515). A canonical in-flight launch has one descriptorrequestId; restore followers only incrementfollowerCount. Cancelling a follower therefore finds no matching record and leaves its request attached to the shared result, while cancelling the leader deletes the workspace and rejects every follower. This violates per-restore abort ownership and can either leak an aborted tab's workspace or tear down a still-live restore. Track the set of restore request IDs on the pending creation and delete only after all owners have cancelled/released (while retaining the existing non-restore sharing rule). -
[P2] Keep path-backed attachments when their inline data exceeds the snapshot cap (
packages/ui/src/stores/client-state-attachments-codec.ts:123-136). File picker attachments include both a usable path and up to 5 MiB ofsource.data, but the codec caps inline data at 64 KiB. BecausehasData && data === undefinedrejects the whole source, a 65 KiB path-backed file loses both its attachment and its exact prompt mention during snapshot normalization. Drop only the oversized bytes and retain{ type: "file", path, mime }; reject the whole attachment only when no restorable path exists. -
[P2] Reserve the global string budget for structural tab fields (
packages/ui/src/stores/client-state-codec.ts:215-280). Each workspace consumes optional drafts and scroll metadata before later tabs consume their requiredfolder/sidecarId, all from one 96 KiB budget. Three valid 32 KiB drafts in the first tab can exhaust the budget and cause every subsequent tab to be filtered out entirely. Normalize or reserve required tab identity fields first, then spend the remaining budget on optional drafts/metadata so optional content cannot delete restorable tabs. -
[P2] Reconcile selection when the remotely deleted session is the active parent (
packages/ui/src/stores/session-api.ts:709-760). Selection cleanup runs only whenactiveSessionId === sessionId. If a root/parent is deleted while one of its children is active,activeParentSessionIdcontinues to point at the removed root and the selected child becomes orphaned in the UI. HandleactiveParentSessionId === sessionIdas well: choose a valid retained ancestor/root fallback or clear both selection IDs atomically.
I excluded two agent suggestions after validation: suppression after explicit clear() is an intentional tested policy, and the restore label shown on web/secondary hosts is presentation-only rather than a merge blocker.
Make Electron and Tauri client-state ownership bounded and shutdown-safe, including Windows session termination flushing, non-fatal Tauri initialization, serialized election release, and PID-reuse-resistant registration acknowledgements. Bound and track remote proxy disposal, surface cleanup failures, and preserve independent restore ownership when canonical workspace launches are shared. Releasing or cancelling one request can no longer tear down a workspace retained by another owner. Preserve authoritative generation status and structural snapshot fields, retain only genuinely path-backed oversized attachments, reconcile deleted parent selections, and correctly report concurrent snapshot write failures. Add regression coverage across Electron, Tauri, server workspace/proxy lifecycle, and UI codec/recovery paths. Validated with 47 Electron tests, 100 server tests, 62 Tauri tests, 51 UI tests, all package typechecks, UI/Electron builds, cargo check, targeted rustfmt, and diff checks.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper follow-up for d43347a6
Rechecked the corrective commit against all ten findings from review 4695368554, including a second independent review pass over the desktop, server, and UI changes. No remaining blocker was found in the corrective diff.
Resolved
- Remote proxy disposal is now tracked across idle cleanup, explicit deletion, creation/shutdown races, and manager shutdown. Active streams are aborted, listener/dispatcher cleanup is force-bounded, and real cleanup failures are surfaced instead of silently discarded.
- Session fetch merging now preserves fields that genuinely changed after capture while retaining authoritative HTTP
working/compactingruntime state and clearing completed admission state. - Tauri registration-timeout markers acknowledge a PID-reuse-resistant registration identity, and marker/primary release is serialized through the bounded registration lock. The zero-primary election and release-gap interleavings are covered.
- Electron marker-backed lock recovery has one finite outer attempt budget, so malformed undeletable locks fall back conservatively instead of blocking the main thread forever.
- Windows
query-session-endnow retains termination for a bounded renderer/native flush, drains primary ownership, stops the managed CLI, deduplicatessession-end, and then exits. - Tauri client-state initialization failures now produce a managed non-primary, restore-disabled state instead of aborting application setup.
- Shared canonical workspace launches track every restore request owner. Leader/follower cancellation detaches only that owner, while release or non-restore adoption makes the workspace durable and prevents a later owner cancellation from deleting it.
- Oversized attachment bytes are removed without dropping a genuinely path-backed attachment or its prompt mention. Inline-only/synthetic files remain rejected rather than being restored as unusable empty attachments.
- Snapshot normalization reserves valid tab identities and active-session selection before optional drafts, scroll data, and layout strings. Malformed tabs cannot consume the structural budget.
- Remote deletion of the selected parent clears parent/child selection atomically, while deletion of the active child still falls back to a retained parent.
The integration pass also fixed an adjacent queue-authority issue in flushClientState(): an in-flight write failure is now re-read after queue settlement, propagated while a clear/disable transaction blocks retry, and cleared only after a successful retry.
Validation
- Electron native suite: 47 passed.
- Server lifecycle/workspace suite: 100 passed.
- Tauri Rust suite: 62 passed.
- UI persistence/recovery suite: 51 passed.
- Server, UI, and Electron typechecks passed.
- UI and Electron production builds passed.
cargo check, targetedrustfmt --check, andgit diff --checkpassed.- Worktree is clean and local/remote heads both resolve to
d43347a63e1dee548447f4dd587db0f25cdfc451. - Controlled agent usage is 18/50 sessions total (14 primary, 4 nested sessions across the full review workflow).
The refreshed GitHub Actions matrix is running at review time. Gatekeeper result: corrective diff clear, subject to CI completion and repository-required independent approval.
Submitted as COMMENTED because GitHub does not allow a PR author to formally approve their own PR.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29357433131 Artifacts expire in 7 days.
|
pascalandr
changed the title
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29360582209 Artifacts expire in 7 days.
|
|
@shantur LGTM |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29366237621 Artifacts expire in 7 days.
|
2 participants
Summary
Implementation
The restored shell state remains local to the desktop client. Durable semantic session properties continue to use OpenCode session metadata separately.
Validation
pm run typecheck --workspace @neuralnomads/codenomad
Platform notes