Skip to content

feat(pr): reconcile remote state after write actions, and never leave the window blank - #221

Merged
huhamhire merged 2 commits into
devfrom
feat/post-action-refresh
Sep 8, 2026
Merged

huhamhire merged 2 commits into
devfrom
feat/post-action-refresh

Conversation

@huhamhire

Copy link
Copy Markdown
Owner

Two related asynchronous-experience problems, both in the same family: an action completes, and the UI is left showing something that is no longer true.

1. Write actions do not settle synchronously

A merge or review verdict returns as soon as the remote accepts it, while the state the UI reads is recomputed asynchronously and lands seconds later:

  • merge — the PR keeps reporting state: ''open'' and keeps coming back in the discovery list. The renderer fired a full poll the instant the merge returned, which is therefore guaranteed to read pre-merge state: a network round-trip across every connection, only to redraw the same list with the PR still in it. It then sat there until the next periodic sync.
  • approve — mergeStatus.canMerge is a server-side verdict over approvals / builds / branch protection. An approval satisfying the last required rule flips it, but the remote recomputes only after the call returns, and nothing re-read it. The merge button stayed hidden until the next periodic sync.

Both actions now start a bounded backoff re-check in main (services/pr-post-action.ts): refresh that one PR on a growing delay until the expected change appears, broadcasting a new prs:changed event for the renderer to reload. Fire-and-forget, so the IPC never waits on the remote settling; past its window the periodic poll remains the backstop.

A confirmed merge archives that one PR directly, via a new Poller.archivePullRequest. The list filters on archivedAt rather than PR state, so a merged PR disappears only once archived — and running a whole poll tick for it would put every connection''s discovery fetch in front of the departure the user is waiting on. The departure a poll infers from absence is already established by the confirmed remote state, so it is asserted directly.

Living in main covers every entry point with one implementation: the buttons, the chat commands, and the CLI''s review write actions all share these controllers.

2. A renderer failure could leave the window permanently blank

Reported as: after a PR action, clicking around quickly turns the UI black, with no recovery. Nothing was watching at any level —

  • main.tsx rendered <App/> with no boundary above it, so any error thrown while rendering unmounted the whole tree and emptied #root;
  • main registered no webContents listeners, so a dead renderer process went unnoticed;
  • and neither kind of crash reached meebox.log — the historical logs contain zero renderer records, which is why the report left nothing behind to diagnose.

Three layers, each covering what the others structurally cannot see:

  1. Render-phase errors → a root ErrorBoundary with a full-window AppCrashScreen (retry / reload). The boundary now also relays the stack via log:write, since the renderer console is not written to file and a caught render error is not an uncaught window error either.
  2. Failures before React mounts → boot-guard.ts, imported first so it is armed before any other module can throw. A module failing while it initializes takes down the entry before render() runs, leaving no React and thus no boundary; the guard paints a plain-DOM recovery screen, depending on no React, i18n or stylesheet, since each of those is a candidate cause.
  3. Renderer process death → render-process-gone / did-fail-load in main, logged and reloaded with a bounded retry budget so a page that dies on load cannot spin in a reload loop. unresponsive is logged but deliberately not recovered: it usually resolves on its own, and reloading would discard in-flight state.

Also fixes a race in the same family, and in exactly the reported scenario: mergeSelectedPr compared the selection against the selectedId its callback had closed over, so selecting another PR while the merge round-trip was in flight cleared the PR the user had just opened.

Verification

  • lint / typecheck / test / build all pass. New test covers archivePullRequest: a PR the remote still lists is not dropped by a tick but is by an explicit archive, and repeat / unknown-id calls are no-ops.
  • Not exercised on a live app: verifying the merge path means actually merging a remote PR, and triggering the crash screens means running the desktop app under a deliberate fault — neither was done here. To self-check the containment, throw in any component in dev: the crash panel should replace the black screen, and meebox.log should gain a renderer render error:App line with the component stack.

The crash containment is deliberately containment, not a root-cause fix — the specific trigger for the black screen is still unknown. It will now surface itself: next reproduction leaves a panel on screen and a stack in the log.

🤖 Generated with Claude Code

huhamhire and others added 2 commits September 7, 2026 20:28
A merge or review verdict returns as soon as the remote accepts it, while the
state the UI reads is recomputed asynchronously and lands seconds later: a
merged PR keeps reporting open and keeps coming back in the discovery list, and
mergeStatus.canMerge still holds its pre-verdict value. Refreshing the instant
the action returned therefore read pre-action state and looked like nothing had
happened -- the merged PR sat in the list, and the merge button stayed hidden
after the approval that had just unblocked it, both until the next periodic poll.

Both actions now start a bounded backoff re-check in main: refresh that one PR
on a growing delay until the expected change appears, broadcasting prs:changed
for the renderer to reload. It is fire-and-forget (the IPC never waits on the
remote settling) and past its window the periodic poll remains the backstop.

A confirmed merge archives that one PR directly, via a new Poller method. The
list filters on archivedAt rather than PR state, so a merged PR disappears only
once archived -- and running a whole poll tick for it would put every
connection's discovery fetch in front of the departure the user is waiting on.
The departure a poll infers from absence is already established by the confirmed
remote state, so it is asserted directly. For the same reason the renderer no
longer fires a full poll on merge: that round is guaranteed to read pre-merge
state, spending a round-trip to redraw the same list.

Living in main covers every entry point with one implementation -- the buttons,
the chat commands, and the CLI's review write actions share these controllers.
The single-PR refresh is extracted for reuse, with comment-cache invalidation
made opt-in: a background re-check must not make the open comment / diff panes
re-fetch for a state the user never asked about.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A renderer failure could leave the window on its bare background permanently,
indistinguishable from a hung app and unrecoverable short of a restart, because
nothing was watching at any level: main.tsx rendered <App/> with no boundary
above it, so any error thrown while rendering unmounted the whole tree and
emptied #root; main registered no webContents listeners, so a dead renderer
process went unnoticed; and no crash of either kind reached meebox.log, which is
why such a report leaves nothing behind to diagnose.

Three layers, each covering what the others structurally cannot see:

- render-phase errors -> a root ErrorBoundary with a full-window AppCrashScreen
  (retry / reload), and the boundary now relays the stack via log:write, since
  the renderer console is not written to file and a caught render error is not
  an uncaught window error either;
- failures before React mounts -> boot-guard.ts, imported first so it is armed
  before any other module can throw. A module failing while it initializes takes
  down the entry before render() runs, leaving no React and thus no boundary; the
  guard paints a plain-DOM recovery screen, depending on no React, i18n or
  stylesheet, since each is a candidate cause;
- renderer process death -> render-process-gone / did-fail-load in main, logged
  and reloaded with a bounded retry budget so a page that dies on load cannot
  spin in a reload loop. 'unresponsive' is logged but not recovered: it usually
  resolves on its own and reloading would discard in-flight state.

Also fixes a race in the same family: mergeSelectedPr compared the selection
against the selectedId its callback had closed over, so selecting another PR
while the merge round-trip was in flight cleared the PR the user had just opened.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@huhamhire huhamhire added enhancement New feature or request bug Something isn't working labels Sep 7, 2026
@huhamhire
huhamhire merged commit b7401fb into dev Sep 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant