perf(layout): skip the grid rebuild when nothing visible changes; harden pwsh lookup - #105
Merged
Merged
Conversation
…den pwsh lookup Closes #103 and #104. Both found by the pre-release code review. #103 — RefreshTerminalLayout ran on every ActiveSession change and began with TerminalGrid.Children.Clear(), detaching and reattaching every WebView2 (an HwndHost, so not a cheap layout pass). #93 made that fire on ordinary interaction rather than only sidebar clicks. The obvious fix — "multi-pane layouts don't change on focus, so skip" — is WRONG. GetViewportSessions pages the visible window around the active session once the session count exceeds the slot count, which is exactly the crowded setup this is meant to help (41 sessions, 6 slots). Skipping there would break paging. So the check compares the render itself: layout + the ordered ids that would be placed. Paging still rebuilds because the id list genuinely differs; only identical renders are skipped. The signature also folds in a _sessionUiVersion counter, bumped whenever _sessionUi gains or loses an entry. Without it the skip is unsound: RestartSessionAsync tears out a session's wrapper and builds a NEW one for the same Id, so the id list is unchanged and the grid would keep rendering the old disposed wrapper. Session ids don't identify visual objects. #104 — where.exe proves a NAME resolves, not that it runs. The false positive on Windows is a Store App Execution Alias stub: zero bytes, a reparse point, resolves on PATH, fails on execution. Since #95 merged the two locators this also picks the wrapper for every non-shell SESSION command, so a stub meant every claude session failed to launch where the old probe would have used powershell.exe. Now reads the resolved path back and rejects zero-length or reparse-point hits, keeping the startup win from #95 without the cliff. 7 new tests for IsRunnable, including that malformed paths are rejected rather than throwing — falling back to powershell.exe is always safe. 303/303 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
This was referenced Aug 27, 2026
AThraen
added a commit
that referenced
this pull request
Aug 29, 2026
…108) Clicking a terminal has never made it the active session. #93 added a PreviewMouseLeftButtonDown handler on the pane's host Border, but WebView2 is an HwndHost: mouse input landing on hosted native content does not raise WPF routed events at all, tunnelling ones included. That handler only ever fired for the ~2px ring around the terminal, never for the terminal itself. So #93 only ever worked for the sidebar path, and — after #106 — for typing. Reported as "still won't switch tabs in the left menu when I click and put focus on another window", which is exactly right. The click can only be observed from inside the page, so terminal-init.js now posts an 'activate' message on mousedown (throttled to 300ms, since promotion is idempotent) and TerminalBridge raises PaneActivated. MainViewModel promotes on that and on KeyboardInput through one shared handler. The page-side handler also calls fitAddon.fit(). The grid rebuild that used to run on every activation incidentally forced a layout pass and therefore a re-fit; #105 skips that rebuild when nothing visible changes, so the re-fit has to be explicit now. Without it xterm's column count can drift from what the PTY was told, and redraws land a character off — reported as typing starting one character into Claude's placeholder, leaving a stray leading "T" from 'Try "how do I log an error?"'. Corrected the WPF-side comment, which claimed the handler covered clicks in the pane. It never did. 312/312 pass, 0 warnings. Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
AThraen
added a commit
that referenced
this pull request
Sep 5, 2026
…#113) Reported as initial rendering looking wrong — text wrapping and running together mid-line — which then corrected itself as soon as the view changed. xterm derives its column count from the MEASURED advance width of the font. The first fit() runs immediately after term.open(); if Cascadia Code hasn't loaded yet, xterm measures the fallback's metrics, computes the wrong cols, and reports a width to the PTY that doesn't match what is drawn. The ResizeObserver cannot correct this: the ELEMENT size never changed, only the glyph metrics, so no resize event fires. It stays wrong until something else forces a fit — which is exactly why switching layouts appeared to fix it. The two existing setTimeout fits (50ms, 250ms) are guesses at "fonts are probably ready by now", and are easily too early during a restore with many WebView2s initialising at once. They stay, since they also cover the separate 0x0-container case, but document.fonts.ready is the actual signal. Same treatment where a profile override changes fontFamily/fontSize — that can switch to a face that isn't loaded either, and fit() there had the same problem. Note this got worse recently rather than appearing from nowhere: #105 stopped rebuilding the grid on activation, and that rebuild used to force an incidental re-fit that masked the mismeasurement. 312/312 pass, 0 warnings. Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
AThraen
added a commit
that referenced
this pull request
Sep 6, 2026
* fix: six findings from the v0.7.0 release review Independent review of v0.6.0..HEAD raised 10 findings. This fixes the six that are release-blocking or self-inflicted; #8 (DbGate per-line throughput) and #9 (File.Replace portability) are deferred by agreement. 1. HIGH — DisposeAndWaitForExitAsync guarded on pty.IsRunning, which is `_hProcess != IntPtr.Zero` and only cleared in Dispose. It does NOT mean the child is alive, so a pane whose claude already exited (user typed `exit`, or it crashed) subscribed to Exited AFTER MonitorExitAsync had already raised it, then waited the full 10s for an event that never comes. That compounded with the 15s shutdown budget added in #101: two stale panes consume the whole allowance and every remaining LIVE Claude session is then force-disposed with no exit wait — losing exactly the ~/.claude.json serialization the loop exists to provide. A slow shutdown became a correctness risk. Added PseudoTerminal.HasExited, latched before Exited is raised so a subscriber can never observe "not exited" for a dead process. Caller guards on it and re-checks after subscribing to close the in-between window. 2. MEDIUM — RestartSessionAsync disposed the old session and relaunched immediately, recreating the concurrent-config-writer race that the launch stagger and shutdown loop both exist to prevent, and making --resume read a session index the outgoing process had not finalised. Claude sessions now wait for real exit plus config quiesce; non-Claude keep the cheap teardown. 3. MEDIUM — SessionType_Changed cleared NameBox unconditionally. In edit mode that silently wrote Name = "" (AutoFillName has nothing to refill from for a remote session). Now returns early in edit mode. 4. MEDIUM — no working-folder validation in edit mode. An empty folder was persisted and LaunchSessionAsync silently fell back to %USERPROFILE%, so a session appeared reconfigured while running somewhere else, with git info and accent colour keyed off an empty path. Flipping Remote -> Local hit this every time. Validated in edit mode only; create mode keeps the useful home-folder default. 5. MEDIUM — self-inflicted in #105. IsRunnable rejected any zero-byte reparse point to skip Store App Execution Alias stubs, but a WORKING Store install of PowerShell 7 is exactly that shape. Store-PS7 users were silently downgraded to 5.1, losing the profile functions that are the entire reason for preferring pwsh — on every session launch, since the locators merged. Traded a rare failure for a common one. Now: metadata fast path for ordinary executables (no spawn), and an actual bounded execution probe only for the ambiguous alias shape. 6. LOW — self-inflicted in #110. RegisterWaitForSingleObject's callback can run before the assignment to `registration` completes when the handle is already signalled, leaving the wait unregistered and disposing the event under a live registration. Published through a lock with a once-only release flag. Also corrected a stale comment in MainViewModel describing a mouse-report filter that no longer exists, and added the _sessionUiVersion bump in RestartSessionAsync that the field's own doc says belongs there. 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be * docs: record the HasExited rule, the Claude restart wait, and why an alias shape is ambiguous Follow-up to the six release-review fixes. - Waiting for a PTY: check HasExited, never IsRunning. Written down because the distinction is invisible at the call site and getting it wrong turned the shutdown budget into a correctness risk rather than a latency guard. - RestartSessionAsync waits for real exit + config quiesce for Claude sessions. - PwshLocator: a zero-byte reparse point is AMBIGUOUS, not bad. A working Store install of PowerShell 7 is the same shape as a dead stub, so only that case is settled by probing. Recorded because rejecting the shape looks obviously correct and silently downgraded Store users. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be * fix(profile): don't let a malformed colour scheme abort a session launch From the release security audit — reported below its severity bar, but it is a real robustness bug. ApplyProfileOverrides called JsonSerializer.Deserialize<JsonElement> on ProfileColorSchemeJson with no guard. That value is normally produced by SchemeMapper, but ImportExportService deserializes a whole AppState from any file the user opens, so it can be arbitrary. A JsonException there propagates out of the launch path and takes down an otherwise fine session. Now caught and logged; the session launches with the default palette. Losing a theme is survivable, failing to start is not. The audit found nothing at or above its bar across the whole v0.6.0..HEAD diff: the PostRunUrl scheme guard holds (no parse divergence between Uri and ShellExecute), SQL parameterisation is intact including the FTS5 and LIKE paths, and nothing is interpolated into the page — host->page traffic is exclusively PostWebMessageAsString of serialized JSON, with no ExecuteScriptAsync or NavigateToString anywhere. One hardening suggestion deliberately NOT taken: PwshLocator validates the absolute path from where.exe then returns the bare name for CreateProcess to re-resolve. Returning the validated path would close a currently-unreachable search-order gap, but the value is interpolated into a command line by BuildCmdLine without quoting, so an absolute path containing spaces would break every wrapped session. Not worth that risk immediately before a release for a gap the audit itself judged unreachable. 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be * fix: address the verification pass on the release-review fixes An independent verification of the six fixes confirmed four outright and found three problems, one of which silently defeated the fix it was checking. FUNCTIONAL — RestartSessionAsync branched on the WRONG command. EditSessionAsync calls SessionConfigEditor.Apply BEFORE RestartSessionAsync, and Apply mutates session.Command in place. So the branch read the NEW command: editing a Claude session to something else tore down the running claude.exe with a bare Dispose, no exit wait and no config quiesce — exactly the ~/.claude.json race the fix was written to close, still open on the one edit that needed it. The old command is now captured pre-Apply and passed in. UX — the sidebar row vanished for up to 11s during a Claude restart. The row is removed early, and the new exit wait was inserted BEFORE the placeholder that exists to stop the row blinking out. Placeholder moved above the wait. PERF — PwshLocator.Executable is a Lazy first forced from PseudoTerminal.Start, which runs on the UI thread. With the Store-alias probe behind it that is up to ~7s of frozen window on first launch, and a realistic ~1s during restore. This is the same class of UI-thread stall #107 and #110 spent four rounds removing, and I reintroduced it. Now warmed on the pool in OnLoaded, so the Lazy is already resolved before any session starts. Also from the verification: - HasExited is now a volatile field. It was safe as an auto-property only because the field-like Exited event's add accessor is an Interlocked CAS and supplied the fence — too subtle to rely on for a future reader that polls. - The setOptions re-fit uses requestAnimationFrame. Neither fonts API works here: fonts.ready is permanently resolved after page load, and fonts.load only matches CSS-connected FontFace objects — there are no @font-face rules and the families are OS-installed, so it resolved having matched nothing. rAF fires after the style change is applied and measured. - Corrected the PwshLocator class doc, which still argued against the probe the code now performs, and a comment in ApplyEditMode that described NameBox being blanked and repaired. Verified correct without change: HasExited latching on every exit path (including the DuplicateHandle early return), the guard/re-check ordering, the edit-mode early return, the folder validation branch and placement, and the WaitForHandleAsync lock (no inline dispatch, so no deadlock; Dispose strictly after Unregister). 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be * fix(startup): await the pwsh warm-up instead of racing it Third verification pass: corrections A (restart command branch) and B (sidebar placeholder) verified correct; C was incomplete. Starting the warm task in OnLoaded is necessary but not sufficient. PwshLocator's Lazy uses ExecutionAndPublication, so a UI thread reaching .Value while the pool thread is still inside the factory takes the Lazy's monitor and blocks for the REMAINING factory duration. That converts a guaranteed stall into a raced one and shrinks it by the head start — it does not remove it. Residual was quantified at up to ~5-6.5s in the pathological case (slow where.exe plus a live Store alias paying a full PS7 cold start). PublicationOnly would not have fixed it either: the UI thread would stop blocking on the monitor and simply run its own copy of the factory, paying the same cost. The task is now awaited before the restore loop. Awaiting yields rather than blocking, so the window stays responsive, and Task.WhenAny with an 8s ceiling — above Resolve's own 7s bound of 2s where.exe + 5s alias probe — means a wedged probe delays restore rather than preventing it. In the ordinary MSI case Resolve finishes in ~10-50ms and this is a no-op. Also dropped a superseded comment in terminal-init.js: the old fonts.load rationale was left above the new requestAnimationFrame one and directly contradicted it. Verified correct without change in this pass: launchedCommand is captured genuinely pre-Apply with no re-mutation before use and only one call site exists; the placeholder is added exactly once and, critically, still AFTER the _sessionUi and _vm.Sessions removals so RebuildSidebarOrder's Resolve takes the placeholder branch rather than a stale live item; HasExited is latched on all three exit paths through the outer finally; and the rAF re-fit cannot throw. 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #103 and #104. Both found by the pre-release code review.
#103 — grid rebuild on every focus change
RefreshTerminalLayoutran on everyActiveSessionchange and began withTerminalGrid.Children.Clear(), detaching and reattaching everyWebView2— anHwndHost, so not a cheap layout pass. #93 made that fire on ordinary interaction rather than only sidebar clicks.The obvious fix is wrong. "Multi-pane layouts don't change on focus, so skip" fails because
GetViewportSessionspages the visible window around the active session once the session count exceeds the slot count — which is exactly the crowded setup this is meant to help (41 sessions, 6 slots). Skipping there would silently break paging.So the check compares the render itself: layout + the ordered ids that would be placed. Paging still rebuilds because the id list genuinely differs; only identical renders are skipped.
The signature also folds in a
_sessionUiVersioncounter, bumped whenever_sessionUigains or loses an entry. Without it the skip is unsound:RestartSessionAsync(edit-session) tears out a session's wrapper and builds a new one for the same Id, so the id list is unchanged, the signature would match, and the grid would keep rendering the old disposed wrapper. Session ids don't identify visual objects.#104 —
where.exeproves a name, not an executableThe false positive on Windows is a Microsoft Store App Execution Alias stub: zero bytes, a reparse point, resolves fine on PATH, fails on execution.
Since #95 merged the two locators, this also picks the wrapper for every non-shell session command — so on such a machine every
claudesession fails to launch, where the old probe would have degraded topowershell.exe. A cliff, not a degradation, and silent until someone starts a session.Now reads the resolved path back and rejects zero-length or reparse-point hits. Keeps the startup win from #95 (no PowerShell spawn on the restore path) without the failure mode.
Tests
7 new for
IsRunnable, including that malformed paths are rejected rather than throwing — falling back topowershell.exeis always safe, propagating an exception is not.Worth a look when testing
#103 is the one to exercise by hand: switch panes in a crowded multi-pane layout (more sessions than slots) and confirm paging still follows the active session, then edit a session's config and confirm the restarted pane renders rather than showing a stale one.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be