fix(ok): deliver menu actions sent before the first render (PRD-8245) (#3784) - #1367
Merged
Conversation
… (#3784) * fix(ok): deliver menu actions sent before the first render (PRD-8245) The menu bar lives in the main process and is live from the moment a window opens. The renderer's listener is not: local-menu-action-bus installs its single onMenuAction forwarder lazily, on first subscribe, which happens inside a React effect and therefore only after the first commit. Anything main pushed on ok:menu-action in between landed on a channel with no listener, and an Electron IPC event with no listener is not queued or retried. It is gone. On a cold or loaded machine that gap is seconds wide, which is exactly when someone reaches for CmdOrCtrl+J. The terminal simply does not open. The preload runs before any renderer script, so it subscribes there instead and holds what arrives until someone attaches, then replays in order. Every OkMenuAction is a payload-free user intent, so honouring one a beat late is what was asked for. The queue is bounded at 32 because a window type that never subscribes can still be main's chosen target. Why this surfaced now: the desktop smoke suite hid it. revealTerminalSurface re-clicks the View menu item until the panel appears, which turns a dropped action into a merely slow one. The Linux terminal gates opened on 2026-08-19 and the terminal specs began running on ubuntu-latest for the first time, where the crossbuild failure rate went from 14.5% to 32.9% over the next two days. The single test that cannot retry, because it is timing the toggle, ate the drop as a bare 5s waitForSelector timeout against a healthy app. Also in this change, from reading the uploaded Playwright traces of those runs: * The toggle-latency test measured the harness, not the app. Its stopwatch started while the renderer was still booting, and on non-darwin it enclosed an editor-window rediscovery poll plus an extra renderer round trip that macOS never pays. Of a 3968ms "toggle" against a 2000ms budget, 413ms was the rediscovery and 1993ms was a dispatch waiting on a busy main thread. It now waits for the renderer to answer promptly before starting the clock, and hands the page it already holds to the dispatch. * New Terminal Window was invoked before main had registered the window's project context. windowsByPath.set lands only after loadFile settles, while the window poll returns as soon as preload answers, so the command resolved no project and opened a HOME-cwd window with an empty collabUrl. * The scrollback retention assertions read xterm's .xterm-accessibility node, which is a screen-reader announcement buffer rather than scrollback. Under a burst it collapses to "Too much output to announce" and truncates mid-token, so the assertion lost a marker while the terminal was perfectly healthy. They read .xterm-rows now, and scroll in half-viewport steps so two adjacent lines cannot straddle a step. One hazard this had to be careful about. Once dropped clicks become real, a retry fired while a queued dispatch is still in flight lands a SECOND toggle and hides the surface it was asked to reveal. The reveal helpers were written on the old assumption that a click could simply vanish, so they now observe before acting and wait long enough to outlast a queued replay. A blanket "wait for a responsive renderer" prelude in every openTerminal helper was tried first. It fixed that, and broke the rail-admission test: the right terminal column settled at its 324px drag floor instead of its 740px preferred width, 2 of 4 runs against 4 of 4 on base. So the readiness barrier is used only where an assertion genuinely needs a settled app, which is the toggle latency measurement and New Terminal Window. * fix(ok): bound what the menu-action queue holds (PRD-8245) Review findings on the menu-action buffer, all of them about what the queue should refuse to hold. Destructive actions are no longer buffered at all. close-active-tab-or-window, kill-terminal, delete and move-to-trash resolve their target when they RUN, so replaying one after the window finally mounts aims it at whatever is selected by then rather than at what the user was looking at when they pressed it. Dropping an early kill-terminal costs a keystroke; honouring it late can cost a session. They still deliver live. A repeated show-or-hide action now collapses to the one intent it expresses. While buffered the user gets no feedback at all, so pressing CmdOrCtrl+J three times is one request restated, not open-close-open. Replaying all three left the terminal shut, which is the exact complaint this whole change exists to fix. Collapsed only against the immediately preceding entry, so an interleaved A,B,A stays three intents and additive actions keep their multiplicity. The replay batch is also put back rather than dropped when the listener set empties during the microtask hop, since "queued or delivered, never lost" is the buffer's whole contract. And the JSDoc now names registerPendingDelivery and says why that one-shot dom-ready gate does not cover recurring actions. Two test findings from the same review: * readShellPid parsed a pid out of xterm's accessibility buffer. It was the one reader here that PARSES rather than substring-matching a whole marker, which makes truncation dangerous instead of merely slow: a MARKER=67818 cut short yields 67, clears the greater-than-zero gate, and is then compared against the real pid as proof a session survived a move. Reads the rows now. * Two tests asserted a layout the app only owes at the width they asked for, while their own innerWidth poll happily caught the beat before the window manager clamped the window back to fit the display. Probing it directly on a settled window: at 1900px the right terminal column resolves to its 740px preferred width, and the 324px drag floor only appears when the window really did shrink to 1510. The app was right; the precondition had lapsed. Both now sample until the width settles, and skip with the display's work area in the reason when it cannot hold the window, the same way the suite skips a platform without a PTY. That poll shape is also the likeliest reading of the restore assertion that has been failing by hundreds of pixels rather than the twenty it allows: a window that shrank mid-test cannot give back the width it was asked to restore. * fix(ok): keep the harness out of the move budget (PRD-8245) The move-placement budget was measuring the test harness, the same way the toggle-latency budget was. Its stopwatch starts inside the renderer when the settlement observer arms, and the dispatch that follows re-discovered the editor window before sending, so an editor-window rediscovery poll sat inside the measured window on every non-darwin run. On darwin the menu click happens inside main and never pays it, so one assertion meant two different things per platform. It ran 540.9ms against a 300ms budget on ubuntu CI, of which the poll is the measured ~400ms. Every caller that times a dispatch now hands over the page it already holds; the rapid-settlement path needed it most, since it was paying that poll seven times inside one settlement. Second review round on the buffer. The two classification sets were hand-maintained ReadonlySet<OkMenuAction>, which type-checks against any subset including the empty one. A new action added to the union would have compiled clean and landed in the default buffer-and-replay bucket, which is exactly what must never happen to a future destructive action. They are now one total Record keyed by the union, so a new member fails the typecheck until somebody classifies it. Verified by deleting kill-terminal from the record: tsc names the missing key. The app package already guards this union with a drift check on OK_MENU_ACTIONS. A record goes one step further than that pattern, and deliberately: drift detection tells you an id is unclassified, a total record refuses to build without a decision. Worth the extra verbosity for a classification whose wrong default destroys a session. Also from the review: * Both dispatch loops now isolate a throwing listener, matching the contract the renderer-side bus already keeps for its own subscribers. In the replay path this is load bearing rather than tidy: the batch is spliced out of the queue before delivery, so an escaping throw silently discarded every intent after it, which is the loss the buffer exists to prevent. * A destructive action dropped with nothing listening now leaves a debug line. It is the one drop a user might report, since they pressed something and saw nothing happen. The parity collapse stays quiet on purpose: it is the designed common path and would log on every repeated keypress. * widenEditorWindow skipped whenever the window settled short, and attributed it to the display without checking. A genuine window-sizing regression on a wide enough display would have been converted into a skip reading "your display is too small". It now compares the work area first and only skips when the display really cannot hold the window; anything else fails. * The replay tests passed whether or not the drain was deferred. A pre-settle assertion pins the microtask hop, which exists so the drain cannot re-enter the React effect that is still subscribing. * Two comments overstated what they described: the scroll step is shorter than a wheel-page rather than half a viewport, and the reveal helper's real defence is its observe-before-acting check, not the length of the window after it. GitOrigin-RevId: 37c4b5ea2b22ec52aaa184f807da61c286e6cd88
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.
Copybara-translated 1 Inkeep OSS change. Rebase-merge this PR so the prepared commit lands directly on public main.
Linear: this mirror replays a change that already merged upstream, so it must not drive ticket status.
skip PRD-8245